loplugin:oncevar

Change-Id: I44fb6858eeff14fcbd9fdfbbb0aabd1433b6a27d
Reviewed-on: https://gerrit.libreoffice.org/30668
Tested-by: Jenkins <ci@libreoffice.org>
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

1 files changed, 81 insertions, 60 deletions

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 <string>
 #include <iostream>
-#include <map>
+#include <unordered_map>
 
 #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<OnceVar>, 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<SourceLocation,int> maVarUsesMap;
-    std::map<SourceLocation,SourceRange> maVarDeclSourceRangeMap;
-    std::map<SourceLocation,SourceRange> maVarUseSourceRangeMap;
     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)
@@ -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<VarDecl>(decl) || isa<ParmVarDecl>(decl)) {
         return true;
     }
     const VarDecl * varDecl = dyn_cast<VarDecl>(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<CXXConstructExpr>(varDecl->getInit());
-    if (!constructExpr || constructExpr->getNumArgs() < 1) {
+    if (!varDecl->hasInit()) {
         return true;
     }
-    if (!isa<StringLiteral>(constructExpr->getArg(0))) {
-        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:
@@ -122,8 +140,11 @@ bool OnceVar::VisitDeclRefExpr( const DeclRefExpr* declRefExpr )
     // and
     //      foo(&xxx);
     // where we cannot inline the declaration.
-    if (isa<MemberExpr>(parentStmt(declRefExpr))
-        || isa<UnaryOperator>(parentStmt(declRefExpr))) {
+    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;
     }