diff options
| -rw-r--r-- | compilerplugins/clang/oncevar.cxx | 288 | ||||
| -rw-r--r-- | compilerplugins/clang/store/oncevar.cxx | 166 | ||||
| -rw-r--r-- | compilerplugins/clang/test/oncevar.cxx | 54 |
3 files changed, 342 insertions, 166 deletions
diff --git a/compilerplugins/clang/oncevar.cxx b/compilerplugins/clang/oncevar.cxx new file mode 100644 index 000000000000..d6c73e32b84b --- /dev/null +++ b/compilerplugins/clang/oncevar.cxx @@ -0,0 +1,288 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +#include <string> +#include <iostream> +#include <unordered_map> +#include <unordered_set> + +#include "plugin.hxx" +#include "check.hxx" +#include "clang/AST/CXXInheritance.h" + +// Original idea from tml. +// Look for variables that are (a) initialised from zero or one constants. (b) only used in one spot. +// In which case, we might as well inline it. + +namespace +{ + +bool startsWith(const std::string& rStr, const char* pSubStr) { + return rStr.compare(0, strlen(pSubStr), pSubStr) == 0; +} + +class OnceVar: + public RecursiveASTVisitor<OnceVar>, public loplugin::Plugin +{ +public: + explicit OnceVar(InstantiationData const & data): Plugin(data) {} + + virtual void run() override { + // ignore some files with problematic macros + std::string fn( compiler.getSourceManager().getFileEntryForID( + compiler.getSourceManager().getMainFileID())->getName() ); + normalizeDotDotInFilePath(fn); + // platform-specific stuff + if (fn == SRCDIR "/sal/osl/unx/thread.cxx" + || fn == SRCDIR "/sot/source/base/formats.cxx" + || fn == SRCDIR "/svl/source/config/languageoptions.cxx" + || fn == SRCDIR "/sfx2/source/appl/appdde.cxx" + || fn == SRCDIR "/configmgr/source/components.cxx" + || fn == SRCDIR "/embeddedobj/source/msole/oleembed.cxx") + return; + // some of this is necessary + if (startsWith( fn, SRCDIR "/sal/qa/")) + return; + if (startsWith( fn, SRCDIR "/comphelper/qa/")) + return; + // TODO need to check calls via function pointer + if (fn == SRCDIR "/i18npool/source/textconversion/textconversion_zh.cxx" + || fn == SRCDIR "/i18npool/source/localedata/localedata.cxx") + return; + // debugging stuff + if (fn == SRCDIR "/sc/source/core/data/dpcache.cxx" + || fn == SRCDIR "/sw/source/core/layout/dbg_lay.cxx" + || fn == SRCDIR "/sw/source/core/layout/ftnfrm.cxx") + return; + // TODO taking local reference to variable + if (fn == SRCDIR "/sc/source/filter/excel/xechart.cxx") + return; + TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); + + for (auto const & varDecl : maVarDeclSet) + { + if (maVarDeclToIgnoreSet.find(varDecl) != maVarDeclToIgnoreSet.end()) + continue; + int noUses = 0; + auto it = maVarUsesMap.find(varDecl); + if (it != maVarUsesMap.end()) + noUses = it->second; + if (noUses > 1) + continue; + report(DiagnosticsEngine::Warning, + "var used only once, should be inlined or declared const", + varDecl->getLocation()) + << varDecl->getSourceRange(); + if (it != maVarUsesMap.end()) + report(DiagnosticsEngine::Note, + "used here", + maVarUseSourceRangeMap[varDecl].getBegin()) + << maVarUseSourceRangeMap[varDecl]; + } + } + + bool VisitDeclRefExpr( const DeclRefExpr* ); + bool VisitVarDecl( const VarDecl* ); + +private: + StringRef getFilename(SourceLocation loc); + + std::unordered_set<VarDecl const *> maVarDeclSet; + std::unordered_set<VarDecl const *> maVarDeclToIgnoreSet; + std::unordered_map<VarDecl const *, int> maVarUsesMap; + std::unordered_map<VarDecl const *, SourceRange> maVarUseSourceRangeMap; +}; + +StringRef OnceVar::getFilename(SourceLocation loc) +{ + SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(loc); + StringRef name { compiler.getSourceManager().getFilename(spellingLocation) }; + return name; +} + +bool OnceVar::VisitVarDecl( const VarDecl* varDecl ) +{ + if (ignoreLocation(varDecl)) { + return true; + } + if (varDecl->isExceptionVariable() || isa<ParmVarDecl>(varDecl)) { + return true; + } + // ignore stuff in header files (which should really not be there, but anyhow) + if (!compiler.getSourceManager().isInMainFile(varDecl->getLocation())) { + return true; + } + // Ignore macros like FD_ZERO + if (compiler.getSourceManager().isMacroBodyExpansion(varDecl->getLocStart())) { + return true; + } + if (varDecl->hasGlobalStorage()) { + return true; + } + auto const tc = loplugin::TypeCheck(varDecl->getType()); + if (!varDecl->getType()->isScalarType() + && !varDecl->getType()->isBooleanType() + && !varDecl->getType()->isEnumeralType() + && !tc.Class("OString").Namespace("rtl").GlobalNamespace() + && !tc.Class("OUString").Namespace("rtl").GlobalNamespace() + && !tc.Class("OStringBuffer").Namespace("rtl").GlobalNamespace() + && !tc.Class("OUStringBuffer").Namespace("rtl").GlobalNamespace()) + { + return true; + } + if (varDecl->getType()->isPointerType()) + return true; + // if it's declared const, ignore it, it's there to make the code easier to read + if (tc.Const()) + return true; + + if (!varDecl->hasInit()) + return true; + + // check for string or scalar literals + bool foundStringLiteral = false; + const Expr * initExpr = varDecl->getInit(); + if (auto e = dyn_cast<ExprWithCleanups>(initExpr)) { + initExpr = e->getSubExpr(); + } + if (auto stringLit = dyn_cast<clang::StringLiteral>(initExpr)) { + foundStringLiteral = true; + // ignore long literals, helps to make the code more legible + if (stringLit->getLength() > 40) { + return true; + } + } else if (auto constructExpr = dyn_cast<CXXConstructExpr>(initExpr)) { + if (constructExpr->getNumArgs() > 0) { + auto stringLit2 = dyn_cast<clang::StringLiteral>(constructExpr->getArg(0)); + foundStringLiteral = stringLit2 != nullptr; + // ignore long literals, helps to make the code more legible + if (stringLit2 && stringLit2->getLength() > 40) { + return true; + } + } + } + if (!foundStringLiteral + && !varDecl->getInit()->isConstantInitializer(compiler.getASTContext(), false/*ForRef*/)) + { + return true; + } + + maVarDeclSet.insert(varDecl); + + return true; +} + +bool OnceVar::VisitDeclRefExpr( const DeclRefExpr* declRefExpr ) +{ + if (ignoreLocation(declRefExpr)) { + return true; + } + const Decl* decl = declRefExpr->getDecl(); + if (!isa<VarDecl>(decl) || isa<ParmVarDecl>(decl)) { + return true; + } + const VarDecl * varDecl = dyn_cast<VarDecl>(decl)->getCanonicalDecl(); + // ignore stuff in header files (which should really not be there, but anyhow) + if (!compiler.getSourceManager().isInMainFile(varDecl->getLocation())) { + return true; + } + + Stmt const * parent = parentStmt(declRefExpr); + // ignore cases like: + // const OUString("xxx") xxx; + // rtl_something(xxx.pData); + // and + // foo(&xxx); + // where we cannot inline the declaration. + auto const tc = loplugin::TypeCheck(varDecl->getType()); + if (tc.Class("OUString").Namespace("rtl").GlobalNamespace() + && parent && (isa<MemberExpr>(parent) || isa<UnaryOperator>(parent))) + { + maVarDeclToIgnoreSet.insert(varDecl); + return true; + } + + // if we take the address of it, or we modify it, ignore it + if (auto unaryOp = dyn_cast_or_null<UnaryOperator>(parent)) { + UnaryOperator::Opcode op = unaryOp->getOpcode(); + if (op == UO_AddrOf || op == UO_PreInc || op == UO_PostInc + || op == UO_PreDec || op == UO_PostDec) + { + maVarDeclToIgnoreSet.insert(varDecl); + return true; + } + } + + // if we assign it another value, or modify it, ignore it + if (auto binaryOp = dyn_cast_or_null<BinaryOperator>(parent)) { + if (binaryOp->getLHS() == declRefExpr) + { + BinaryOperator::Opcode op = binaryOp->getOpcode(); + if (op == BO_Assign || op == BO_PtrMemD || op == BO_PtrMemI || op == BO_MulAssign + || op == BO_DivAssign || op == BO_RemAssign || op == BO_AddAssign + || op == BO_SubAssign || op == BO_ShlAssign || op == BO_ShrAssign + || op == BO_AndAssign || op == BO_XorAssign || op == BO_OrAssign) + { + maVarDeclToIgnoreSet.insert(varDecl); + return true; + } + } + } + + // ignore those ones we are passing by reference + if (auto callExpr = dyn_cast_or_null<CallExpr>(parent)) { + const FunctionDecl* calleeFunctionDecl = callExpr->getDirectCallee(); + if (calleeFunctionDecl) { + for (unsigned i = 0; i < callExpr->getNumArgs(); ++i) { + if (callExpr->getArg(i) == declRefExpr) { + if (i < calleeFunctionDecl->getNumParams()) { + QualType qt { calleeFunctionDecl->getParamDecl(i)->getType() }; + if (loplugin::TypeCheck(qt).LvalueReference()) { + maVarDeclToIgnoreSet.insert(varDecl); + return true; + } + } + break; + } + } + } + } + // ignore those ones we are passing by reference + if (auto cxxConstructExpr = dyn_cast_or_null<CXXConstructExpr>(parent)) { + const CXXConstructorDecl* cxxConstructorDecl = cxxConstructExpr->getConstructor(); + for (unsigned i = 0; i < cxxConstructExpr->getNumArgs(); ++i) { + if (cxxConstructExpr->getArg(i) == declRefExpr) { + if (i < cxxConstructorDecl->getNumParams()) { + QualType qt { cxxConstructorDecl->getParamDecl(i)->getType() }; + if (loplugin::TypeCheck(qt).LvalueReference()) { + maVarDeclToIgnoreSet.insert(varDecl); + return true; + } + } + break; + } + } + return true; + } + + if (maVarUsesMap.find(varDecl) == maVarUsesMap.end()) { + maVarUsesMap[varDecl] = 1; + maVarUseSourceRangeMap[varDecl] = declRefExpr->getSourceRange(); + } else { + maVarUsesMap[varDecl]++; + } + + return true; +} + +loplugin::Plugin::Registration< OnceVar > X("oncevar", false); + +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/compilerplugins/clang/store/oncevar.cxx b/compilerplugins/clang/store/oncevar.cxx deleted file mode 100644 index 1947515cd749..000000000000 --- a/compilerplugins/clang/store/oncevar.cxx +++ /dev/null @@ -1,166 +0,0 @@ -/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */ -/* - * This file is part of the LibreOffice project. - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. - */ - -#include <string> -#include <iostream> -#include <unordered_map> - -#include "plugin.hxx" -#include "check.hxx" -#include "clang/AST/CXXInheritance.h" - -// Idea from tml. -// Check for OUString/char[] variables that are -// (1) initialised from a string literal -// (2) only used in one spot -// In which case, we might as well inline it. - -namespace -{ - -class OnceVar: - public RecursiveASTVisitor<OnceVar>, public loplugin::Plugin -{ -public: - explicit OnceVar(InstantiationData const & data): Plugin(data) {} - - virtual void run() override { - // ignore some files with problematic macros - std::string fn( compiler.getSourceManager().getFileEntryForID( - compiler.getSourceManager().getMainFileID())->getName() ); - normalizeDotDotInFilePath(fn); - // TODO not possible here, need to figure out how to ignore cases where we index - // into the string - if (fn == SRCDIR "/vcl/source/filter/ixpm/xpmread.cxx") - return; - if (fn == SRCDIR "/sc/source/filter/excel/xiescher.cxx") - return; - // all the constants are nicely lined up at the top of the file, seems - // a pity to just inline a handful. - if (fn == SRCDIR "/sc/source/ui/docshell/docsh.cxx") - return; - if (fn == SRCDIR "/sw/source/core/text/EnhancedPDFExportHelper.cxx") - return; - if (fn == SRCDIR "/svgio/source/svgreader/svgtoken.cxx") - return; - // TODO explicit length array - if (fn == SRCDIR "/sal/qa/osl/file/osl_File.cxx") - return; - - TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); - - for (auto it = maVarUsesMap.cbegin(); it != maVarUsesMap.cend(); ++it) - { - if (it->second == 1) - { - report(DiagnosticsEngine::Warning, - "string/char[] var used only once, should be inlined", - it->first) - << maVarDeclSourceRangeMap[it->first]; - report(DiagnosticsEngine::Note, - "to this spot", - maVarUseSourceRangeMap[it->first].getBegin()) - << maVarUseSourceRangeMap[it->first]; - } - } - } - - bool VisitDeclRefExpr( const DeclRefExpr* ); - -private: - StringRef getFilename(SourceLocation loc); - - struct SourceLocationHash - { - size_t operator()( SourceLocation const & sl ) const - { - return sl.getRawEncoding(); - } - }; - std::unordered_map<SourceLocation, int, SourceLocationHash> maVarUsesMap; - std::unordered_map<SourceLocation, SourceRange, SourceLocationHash> maVarDeclSourceRangeMap; - std::unordered_map<SourceLocation, SourceRange, SourceLocationHash> maVarUseSourceRangeMap; -}; - -StringRef OnceVar::getFilename(SourceLocation loc) -{ - SourceLocation spellingLocation = compiler.getSourceManager().getSpellingLoc(loc); - StringRef name { compiler.getSourceManager().getFilename(spellingLocation) }; - return name; -} - -bool OnceVar::VisitDeclRefExpr( const DeclRefExpr* declRefExpr ) -{ - if (ignoreLocation(declRefExpr)) { - return true; - } - const Decl* decl = declRefExpr->getDecl(); - if (!isa<VarDecl>(decl) || isa<ParmVarDecl>(decl)) { - return true; - } - const VarDecl * varDecl = dyn_cast<VarDecl>(decl)->getCanonicalDecl(); - // ignore stuff in header files (which should really not be there, but anyhow) - if (!compiler.getSourceManager().isInMainFile(varDecl->getLocation())) { - return true; - } - if (!varDecl->hasInit()) { - return true; - } - if (const StringLiteral* stringLit = dyn_cast<StringLiteral>(varDecl->getInit())) { - // ignore long literals, helps to make the code more legible - if (stringLit->getLength() > 40) { - return true; - } - // ok - } else { - const CXXConstructExpr* constructExpr = dyn_cast<CXXConstructExpr>(varDecl->getInit()); - if (!constructExpr || constructExpr->getNumArgs() != 1) { - return true; - } - const StringLiteral* stringLit2 = dyn_cast<StringLiteral>(varDecl->getInit()); - if (!stringLit2) { - return true; - } - // ignore long literals, helps to make the code more legible - if (stringLit2->getLength() > 40) { - return true; - } - } - SourceLocation loc = varDecl->getLocation(); - - // ignore cases like: - // const OUString("xxx") xxx; - // rtl_something(xxx.pData); - // and - // foo(&xxx); - // where we cannot inline the declaration. - auto const tc = loplugin::TypeCheck(varDecl->getType()); - if (tc.Class("OUString").Namespace("rtl").GlobalNamespace() - && (isa<MemberExpr>(parentStmt(declRefExpr)) - || isa<UnaryOperator>(parentStmt(declRefExpr)))) - { - maVarUsesMap[loc] = 2; - return true; - } - - if (maVarUsesMap.find(loc) == maVarUsesMap.end()) { - maVarUsesMap[loc] = 1; - maVarDeclSourceRangeMap[loc] = varDecl->getSourceRange(); - maVarUseSourceRangeMap[loc] = declRefExpr->getSourceRange(); - } else { - maVarUsesMap[loc]++; - } - return true; -} - -loplugin::Plugin::Registration< OnceVar > X("oncevar"); - -} - -/* vim:set shiftwidth=4 softtabstop=4 expandtab: */ diff --git a/compilerplugins/clang/test/oncevar.cxx b/compilerplugins/clang/test/oncevar.cxx new file mode 100644 index 000000000000..214293f185e5 --- /dev/null +++ b/compilerplugins/clang/test/oncevar.cxx @@ -0,0 +1,54 @@ +/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4; fill-column: 100 -*- */ +/* + * This file is part of the LibreOffice project. + * + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. + */ + +#include <rtl/ustring.hxx> + +/*int foo() { return 1; }*/ + +void call_value(int); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}} +void call_const_ref(int const &); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}} +void call_ref(int &); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}} +void call_value(OUString); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}} +void call_const_ref(OUString const &); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}} +void call_ref(OUString &); // expected-error {{extern prototype in main file without definition [loplugin:externandnotdefined]}} + +int main() { +/* TODO + int i; + int x = 2; + if ( (i = foo()) == 0 ) { + x = 1; + } +*/ + + + int i1 = 2; // expected-error {{var used only once, should be inlined or declared const [loplugin:oncevar]}} + call_value(i1); // expected-note {{used here [loplugin:oncevar]}} + int i2 = 2; // expected-error {{var used only once, should be inlined or declared const [loplugin:oncevar]}} + call_const_ref(i2); // expected-note {{used here [loplugin:oncevar]}} + + // don't expect warnings here + int i3; + call_ref(i3); + int const i4 = 2; + call_value(i4); + + OUString s1("xxx"); // expected-error {{var used only once, should be inlined or declared const [loplugin:oncevar]}} + call_value(s1); // expected-note {{used here [loplugin:oncevar]}} + OUString s2("xxx"); // expected-error {{var used only once, should be inlined or declared const [loplugin:oncevar]}} + call_const_ref(s2); // expected-note {{used here [loplugin:oncevar]}} + + // don't expect warnings here + OUString s3; + call_ref(s3); + OUString const s4("xxx"); + call_value(s4); +} + +/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */ |