From 8955c3fde60b0621527e4b91576e49778494f926 Mon Sep 17 00:00:00 2001 From: Noel Grandin Date: Mon, 7 Nov 2016 15:59:37 +0200 Subject: loplugin:oncevar Change-Id: I44fb6858eeff14fcbd9fdfbbb0aabd1433b6a27d Reviewed-on: https://gerrit.libreoffice.org/30668 Tested-by: Jenkins Reviewed-by: Noel Grandin --- compilerplugins/clang/store/oncevar.cxx | 141 ++++++++++++++++++-------------- 1 file changed, 81 insertions(+), 60 deletions(-) (limited to 'compilerplugins') diff --git a/compilerplugins/clang/store/oncevar.cxx b/compilerplugins/clang/store/oncevar.cxx index e618922a2395..1947515cd749 100644 --- a/compilerplugins/clang/store/oncevar.cxx +++ b/compilerplugins/clang/store/oncevar.cxx @@ -9,13 +9,14 @@ #include #include -#include +#include #include "plugin.hxx" +#include "check.hxx" #include "clang/AST/CXXInheritance.h" // Idea from tml. -// Check for OUString variables that are +// 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. @@ -27,21 +28,64 @@ class OnceVar: public RecursiveASTVisitor, public loplugin::Plugin { public: - explicit OnceVar(InstantiationData const & data): Plugin(data), mbChecking(false) {} + 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 TraverseFunctionDecl( FunctionDecl* stmt ); bool VisitDeclRefExpr( const DeclRefExpr* ); private: - bool mbChecking; - std::map maVarUsesMap; - std::map maVarDeclSourceRangeMap; - std::map maVarUseSourceRangeMap; StringRef getFilename(SourceLocation loc); + + struct SourceLocationHash + { + size_t operator()( SourceLocation const & sl ) const + { + return sl.getRawEncoding(); + } + }; + std::unordered_map maVarUsesMap; + std::unordered_map maVarDeclSourceRangeMap; + std::unordered_map maVarUseSourceRangeMap; }; StringRef OnceVar::getFilename(SourceLocation loc) @@ -51,69 +95,43 @@ StringRef OnceVar::getFilename(SourceLocation loc) return name; } -bool OnceVar::TraverseFunctionDecl(FunctionDecl * decl) -{ - if (ignoreLocation(decl)) { - return true; - } - if (!decl->hasBody()) { - return true; - } - if ( decl != decl->getCanonicalDecl() ) { - return true; - } - - // some places, it makes the code worse - StringRef aFileName = getFilename(decl->getLocStart()); - if (aFileName.startswith(SRCDIR "/sal/qa/osl/module/osl_Module.cxx")) { - return true; - } - - maVarUsesMap.clear(); - maVarDeclSourceRangeMap.clear(); - mbChecking = true; - TraverseStmt(decl->getBody()); - mbChecking = false; - for (auto it = maVarUsesMap.cbegin(); it != maVarUsesMap.cend(); ++it) - { - if (it->second == 1) - { - report(DiagnosticsEngine::Warning, - "OUString 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]; - } - } - return true; -} - bool OnceVar::VisitDeclRefExpr( const DeclRefExpr* declRefExpr ) { - if (!mbChecking) + if (ignoreLocation(declRefExpr)) { return true; + } const Decl* decl = declRefExpr->getDecl(); if (!isa(decl) || isa(decl)) { return true; } const VarDecl * varDecl = dyn_cast(decl)->getCanonicalDecl(); - if (!varDecl->hasInit() || varDecl->hasGlobalStorage()) { - return true; - } - if (varDecl->getType().getUnqualifiedType().getAsString().find("OUString") == std::string::npos) { + // ignore stuff in header files (which should really not be there, but anyhow) + if (!compiler.getSourceManager().isInMainFile(varDecl->getLocation())) { return true; } - const CXXConstructExpr* constructExpr = dyn_cast(varDecl->getInit()); - if (!constructExpr || constructExpr->getNumArgs() < 1) { + if (!varDecl->hasInit()) { return true; } - if (!isa(constructExpr->getArg(0))) { - return true; + if (const StringLiteral* stringLit = dyn_cast(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(varDecl->getInit()); + if (!constructExpr || constructExpr->getNumArgs() != 1) { + return true; + } + const StringLiteral* stringLit2 = dyn_cast(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: @@ -122,8 +140,11 @@ bool OnceVar::VisitDeclRefExpr( const DeclRefExpr* declRefExpr ) // and // foo(&xxx); // where we cannot inline the declaration. - if (isa(parentStmt(declRefExpr)) - || isa(parentStmt(declRefExpr))) { + auto const tc = loplugin::TypeCheck(varDecl->getType()); + if (tc.Class("OUString").Namespace("rtl").GlobalNamespace() + && (isa(parentStmt(declRefExpr)) + || isa(parentStmt(declRefExpr)))) + { maVarUsesMap[loc] = 2; return true; } -- cgit