New loplugin:elidestringvar

Quoting compilerplugins/clang/elidestringvar.cxx (and see there for a
rationale):  "Find cases where a variable of a string type (at least for now,
only OUString) is initialized with a literal value (incl. as an empty string)
and used only once."

(This commit includes fixes that become necessary in code changed after the
preceding "Upcoming loplugin:elidestringvar" commits.)

As a follow-up TODO, uses of the

explicit OUString( sal_Unicode value )

ctor where value is char or char16_t literal should be detected too, so that
e.g. one_quote would have been treated like two_quote in
4b85108773f9851f358a4daa8869eeadc638d103 "Upcoming loplugin:elidestringvar: sc"

> --- a/sc/source/core/tool/compiler.cxx
> +++ b/sc/source/core/tool/compiler.cxx
> @@ -1902,9 +1902,8 @@ void ScCompiler::CheckTabQuotes( OUString& rString,
>              if( bNeedsQuote )
>              {
>                  const OUString one_quote('\'');
> -                const OUString two_quote("''");
>                  // escape embedded quotes
> -                rString = rString.replaceAll( one_quote, two_quote );
> +                rString = rString.replaceAll( one_quote, "''" );
>              }
>              break;
>      }

Change-Id: I7b74f1735a07540e3d789df1d14c84081c824b6c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/95696
Tested-by: Jenkins
Reviewed-by: Stephan Bergmann <sbergman@redhat.com>

7 files changed, 438 insertions, 14 deletions

diff --git a/compilerplugins/clang/elidestringvar.cxx b/compilerplugins/clang/elidestringvar.cxx
new file mode 100644
index 000000000000..79aa75b2da8f
--- /dev/null
+++ b/compilerplugins/clang/elidestringvar.cxx
@@ -0,0 +1,431 @@
+/* -*- 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/.
+ */
+
+#ifndef LO_CLANG_SHARED_PLUGINS
+
+#include <algorithm>
+#include <cassert>
+#include <map>
+
+#include "check.hxx"
+#include "compat.hxx"
+#include "plugin.hxx"
+
+// Find cases where a variable of a string type (at least for now, only OUString) is initialized
+// with a literal value (incl. as an empty string) and used only once.  Conservatively this only
+// covers local non-static variables that are not defined outside of the loop (if any) in which they
+// are used, as other cases may deliberately use the variable for performance (or even correctness,
+// if addresses are taken and compared) reasons.
+//
+// For one, the historically heavy syntax for such uses of string literals
+// (RTL_CONSTASCII_USTRINGPARAM etc.) probably explains many of these redundant variables, wich can
+// now be considered cargo-cult baggage.  For another, some of those variables are used as arguments
+// to functions which also have more efficient overloads directly taking string literals.  And for
+// yet another, some cases with default-initialized variables turned out to be effectively unused
+// code that could be removed completely (d073cca5f7c04de3e1bcedda334d864e98ac7835 "Clean up dead
+// code", 91345e7dde6100496a7c9e815b72b2821ae82bc2 "Clean up dead code",
+// 868b0763ac47f765cb48c277897274a595b831d0 "Upcoming loplugin:elidestringvar: dbaccess" in
+// dbaccess/source/ui/app/AppController.cxx, bde0aac4ccf7b830b5ef21d5b9e75e62aee6aaf9 "Clean up dead
+// code", 354aefec42de856b4ab6201ada54a3a3c630b4bd "Upcoming loplugin:elidestringvar: cui" in
+// cui/source/dialogs/SpellDialog.cxx).
+
+namespace
+{
+class ElideStringVar : public loplugin::FilteringPlugin<ElideStringVar>
+{
+public:
+    explicit ElideStringVar(loplugin::InstantiationData const& data)
+        : FilteringPlugin(data)
+    {
+    }
+
+    bool preRun() override { return compiler.getLangOpts().CPlusPlus; }
+
+    void postRun() override
+    {
+        for (auto const& var : vars_)
+        {
+            if (!var.second.singleUse || *var.second.singleUse == nullptr)
+            {
+                continue;
+            }
+            if (containsPreprocessingConditionalInclusion(SourceRange(
+                    compat::getBeginLoc(var.first), compat::getEndLoc(*var.second.singleUse))))
+            {
+                // This is not perfect, as additional uses can be hidden in conditional blocks that
+                // only start after the (would-be) single use (as was the case in
+                // 3bc5057f9689e024957cfa898a221ee2c4c4afe7 "Upcoming loplugin:elidestringvar:
+                // testtools" when built with --enable-debug, but where also fixing the hidden
+                // additional use was trivial).  If this ever becomes a real problem, we can extend
+                // the above check to cover more of the current function body's remainder.
+                continue;
+            }
+            report(DiagnosticsEngine::Warning,
+                   "replace single use of literal OUString variable with the literal",
+                   (*var.second.singleUse)->getExprLoc())
+                << (*var.second.singleUse)->getSourceRange();
+            report(DiagnosticsEngine::Note, "literal OUString variable defined here",
+                   var.first->getLocation())
+                << var.first->getSourceRange();
+        }
+    }
+
+    bool VisitVarDecl(VarDecl const* decl)
+    {
+        if (ignoreLocation(decl))
+        {
+            return true;
+        }
+        if (!decl->isThisDeclarationADefinition())
+        {
+            return true;
+        }
+        if (isa<ParmVarDecl>(decl))
+        {
+            return true;
+        }
+        if (decl->getStorageDuration() != SD_Automatic)
+        {
+            return true;
+        }
+        if (!loplugin::TypeCheck(decl->getType())
+                 .Class("OUString")
+                 .Namespace("rtl")
+                 .GlobalNamespace())
+        {
+            return true;
+        }
+        if (!decl->hasInit())
+        {
+            return true;
+        }
+        auto const e1 = dyn_cast<CXXConstructExpr>(decl->getInit()->IgnoreParenImpCasts());
+        if (e1 == nullptr)
+        {
+            return true;
+        }
+        if (!loplugin::TypeCheck(e1->getType())
+                 .Class("OUString")
+                 .Namespace("rtl")
+                 .GlobalNamespace())
+        {
+            return true;
+        }
+        switch (e1->getNumArgs())
+        {
+            case 0:
+                break;
+            case 2:
+            {
+                auto const e2 = e1->getArg(1);
+                if (!(isa<CXXDefaultArgExpr>(e2)
+                      && loplugin::TypeCheck(e2->getType())
+                             .Struct("Dummy")
+                             .Namespace("libreoffice_internal")
+                             .Namespace("rtl")
+                             .GlobalNamespace()))
+                {
+                    return true;
+                }
+                break;
+            }
+            default:
+                return true;
+        }
+        auto const ok = vars_.emplace(decl, Data(getInnermostLoop()));
+        assert(ok.second);
+        (void)ok;
+        return true;
+    }
+
+    bool VisitDeclRefExpr(DeclRefExpr const* expr)
+    {
+        if (ignoreLocation(expr))
+        {
+            return true;
+        }
+        auto const var = dyn_cast<VarDecl>(expr->getDecl());
+        if (var == nullptr)
+        {
+            return true;
+        }
+        auto const i = vars_.find(var);
+        if (i == vars_.end())
+        {
+            return true;
+        }
+        i->second.singleUse
+            = i->second.singleUse || i->second.innermostLoop != getInnermostLoop() ? nullptr : expr;
+        return true;
+    }
+
+    bool VisitMemberExpr(MemberExpr const* expr)
+    {
+        if (ignoreLocation(expr))
+        {
+            return true;
+        }
+        auto const e = dyn_cast<DeclRefExpr>(expr->getBase()->IgnoreParenImpCasts());
+        if (e == nullptr)
+        {
+            return true;
+        }
+        auto const var = dyn_cast<VarDecl>(e->getDecl());
+        if (var == nullptr)
+        {
+            return true;
+        }
+        auto const i = vars_.find(var);
+        if (i == vars_.end())
+        {
+            return true;
+        }
+        i->second.singleUse = nullptr;
+        return true;
+    }
+
+    bool VisitUnaryOperator(UnaryOperator const* expr)
+    {
+        if (ignoreLocation(expr))
+        {
+            return true;
+        }
+        if (expr->getOpcode() != UO_AddrOf)
+        {
+            return true;
+        }
+        auto const e = dyn_cast<DeclRefExpr>(expr->getSubExpr()->IgnoreParenImpCasts());
+        if (e == nullptr)
+        {
+            return true;
+        }
+        auto const var = dyn_cast<VarDecl>(e->getDecl());
+        if (var == nullptr)
+        {
+            return true;
+        }
+        auto const i = vars_.find(var);
+        if (i == vars_.end())
+        {
+            return true;
+        }
+        i->second.singleUse = nullptr;
+        return true;
+    }
+
+    bool VisitCallExpr(CallExpr const* expr)
+    {
+        if (ignoreLocation(expr))
+        {
+            return true;
+        }
+        auto const fun = expr->getDirectCallee();
+        if (fun == nullptr)
+        {
+            return true;
+        }
+        unsigned const n = std::min(fun->getNumParams(), expr->getNumArgs());
+        for (unsigned i = 0; i != n; ++i)
+        {
+            if (!loplugin::TypeCheck(fun->getParamDecl(i)->getType())
+                     .LvalueReference()
+                     .NonConstVolatile())
+            {
+                continue;
+            }
+            auto const e = dyn_cast<DeclRefExpr>(expr->getArg(i)->IgnoreParenImpCasts());
+            if (e == nullptr)
+            {
+                continue;
+            }
+            auto const var = dyn_cast<VarDecl>(e->getDecl());
+            if (var == nullptr)
+            {
+                continue;
+            }
+            auto const j = vars_.find(var);
+            if (j == vars_.end())
+            {
+                continue;
+            }
+            j->second.singleUse = nullptr;
+        }
+        return true;
+    }
+
+    bool VisitCXXConstructExpr(CXXConstructExpr const* expr)
+    {
+        if (ignoreLocation(expr))
+        {
+            return true;
+        }
+        auto const ctor = expr->getConstructor();
+        unsigned const n = std::min(ctor->getNumParams(), expr->getNumArgs());
+        for (unsigned i = 0; i != n; ++i)
+        {
+            if (!loplugin::TypeCheck(ctor->getParamDecl(i)->getType())
+                     .LvalueReference()
+                     .NonConstVolatile())
+            {
+                continue;
+            }
+            auto const e = dyn_cast<DeclRefExpr>(expr->getArg(i)->IgnoreParenImpCasts());
+            if (e == nullptr)
+            {
+                continue;
+            }
+            auto const var = dyn_cast<VarDecl>(e->getDecl());
+            if (var == nullptr)
+            {
+                continue;
+            }
+            auto const j = vars_.find(var);
+            if (j == vars_.end())
+            {
+                continue;
+            }
+            j->second.singleUse = nullptr;
+        }
+        return true;
+    }
+
+    bool TraverseWhileStmt(WhileStmt* stmt)
+    {
+        bool ret = true;
+        if (PreTraverseWhileStmt(stmt))
+        {
+            ret = FilteringPlugin::TraverseWhileStmt(stmt);
+            PostTraverseWhileStmt(stmt, ret);
+        }
+        return ret;
+    }
+
+    bool PreTraverseWhileStmt(WhileStmt* stmt)
+    {
+        innermostLoop_.push(stmt);
+        return true;
+    }
+
+    bool PostTraverseWhileStmt(WhileStmt* stmt, bool)
+    {
+        assert(!innermostLoop_.empty());
+        assert(innermostLoop_.top() == stmt);
+        innermostLoop_.pop();
+        return true;
+    }
+
+    bool TraverseDoStmt(DoStmt* stmt)
+    {
+        bool ret = true;
+        if (PreTraverseDoStmt(stmt))
+        {
+            ret = FilteringPlugin::TraverseDoStmt(stmt);
+            PostTraverseDoStmt(stmt, ret);
+        }
+        return ret;
+    }
+
+    bool PreTraverseDoStmt(DoStmt* stmt)
+    {
+        innermostLoop_.push(stmt);
+        return true;
+    }
+
+    bool PostTraverseDoStmt(DoStmt* stmt, bool)
+    {
+        assert(!innermostLoop_.empty());
+        assert(innermostLoop_.top() == stmt);
+        innermostLoop_.pop();
+        return true;
+    }
+
+    bool TraverseForStmt(ForStmt* stmt)
+    {
+        bool ret = true;
+        if (PreTraverseForStmt(stmt))
+        {
+            ret = FilteringPlugin::TraverseForStmt(stmt);
+            PostTraverseForStmt(stmt, ret);
+        }
+        return ret;
+    }
+
+    bool PreTraverseForStmt(ForStmt* stmt)
+    {
+        innermostLoop_.push(stmt);
+        return true;
+    }
+
+    bool PostTraverseForStmt(ForStmt* stmt, bool)
+    {
+        assert(!innermostLoop_.empty());
+        assert(innermostLoop_.top() == stmt);
+        innermostLoop_.pop();
+        return true;
+    }
+
+    bool TraverseCXXForRangeStmt(CXXForRangeStmt* stmt)
+    {
+        bool ret = true;
+        if (PreTraverseCXXForRangeStmt(stmt))
+        {
+            ret = FilteringPlugin::TraverseCXXForRangeStmt(stmt);
+            PostTraverseCXXForRangeStmt(stmt, ret);
+        }
+        return ret;
+    }
+
+    bool PreTraverseCXXForRangeStmt(CXXForRangeStmt* stmt)
+    {
+        innermostLoop_.push(stmt);
+        return true;
+    }
+
+    bool PostTraverseCXXForRangeStmt(CXXForRangeStmt* stmt, bool)
+    {
+        assert(!innermostLoop_.empty());
+        assert(innermostLoop_.top() == stmt);
+        innermostLoop_.pop();
+        return true;
+    }
+
+private:
+    void run() override
+    {
+        if (preRun() && TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()))
+        {
+            postRun();
+        }
+    }
+
+    Stmt const* getInnermostLoop() const
+    {
+        return innermostLoop_.empty() ? nullptr : innermostLoop_.top();
+    }
+
+    struct Data
+    {
+        Data(Stmt const* theInnermostLoop)
+            : innermostLoop(theInnermostLoop)
+        {
+        }
+        Stmt const* innermostLoop;
+        llvm::Optional<Expr const*> singleUse;
+    };
+
+    std::stack<Stmt const*> innermostLoop_;
+    std::map<VarDecl const*, Data> vars_;
+};
+
+loplugin::Plugin::Registration<ElideStringVar> elidestringvar("elidestringvar");
+}
+
+#endif
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/sharedvisitor/dummyplugin.hxx b/compilerplugins/clang/sharedvisitor/dummyplugin.hxx
index 32e0e5e560ee..b52dfaebd238 100644
--- a/compilerplugins/clang/sharedvisitor/dummyplugin.hxx
+++ b/compilerplugins/clang/sharedvisitor/dummyplugin.hxx
@@ -41,6 +41,7 @@ public:
     bool TraverseWhileStmt( WhileStmt* ) { return complain(); }
     bool TraverseDoStmt( DoStmt* ) { return complain(); }
     bool TraverseForStmt( ForStmt* ) { return complain(); }
+    bool TraverseCXXForRangeStmt( CXXForRangeStmt* ) { return complain(); }
     bool TraverseUnaryLNot( UnaryOperator* ) { return complain(); }
     bool TraverseBinLAnd( BinaryOperator* ) { return complain(); }
     bool TraverseBinLOr( BinaryOperator* ) { return complain(); }
diff --git a/extensions/source/propctrlr/selectlabeldialog.cxx b/extensions/source/propctrlr/selectlabeldialog.cxx
index a1ddce7b10b4..bc1d16111c14 100644
--- a/extensions/source/propctrlr/selectlabeldialog.cxx
+++ b/extensions/source/propctrlr/selectlabeldialog.cxx
@@ -104,10 +104,9 @@ namespace pcr
 
             // insert the root
             OUString sRootName(PcrRes(RID_STR_FORMS));
-            OUString aFormImage(RID_EXTBMP_FORMS);
             m_xControlTree->insert(nullptr, -1, &sRootName, nullptr,
                                    nullptr, nullptr, false, m_xScratchIter.get());
-            m_xControlTree->set_image(*m_xScratchIter, aFormImage);
+            m_xControlTree->set_image(*m_xScratchIter, RID_EXTBMP_FORMS);
 
             // build the tree
             m_xInitialSelection.reset();
@@ -179,11 +178,9 @@ namespace pcr
                 Reference< XIndexAccess >  xCont(xAsSet, UNO_QUERY);
                 if (xCont.is() && xCont->getCount())
                 {   // yes -> step down
-                    OUString aFormImage(RID_EXTBMP_FORM);
-
                     m_xControlTree->insert(&rContainerEntry, -1, &sName, nullptr,
                                            nullptr, nullptr, false, m_xScratchIter.get());
-                    m_xControlTree->set_image(*m_xScratchIter, aFormImage);
+                    m_xControlTree->set_image(*m_xScratchIter, RID_EXTBMP_FORM);
                     auto xIter = m_xControlTree->make_iterator(&rContainerEntry);
                     m_xControlTree->iter_nth_child(*xIter, nChildren);
                     sal_Int32 nContChildren = InsertEntries(xCont, *xIter);
diff --git a/fpicker/source/office/foldertree.cxx b/fpicker/source/office/foldertree.cxx
index c2c9c764a742..9e618bd9d0b9 100644
--- a/fpicker/source/office/foldertree.cxx
+++ b/fpicker/source/office/foldertree.cxx
@@ -45,10 +45,9 @@ IMPL_LINK(FolderTree, RequestingChildrenHdl, const weld::TreeIter&, rEntry, bool
 
 void FolderTree::InsertRootEntry(const OUString& rId, const OUString& rRootLabel)
 {
-    OUString sFolderImage(RID_BMP_FOLDER);
     m_xTreeView->insert(nullptr, -1, &rRootLabel, &rId, nullptr, nullptr,
                         true, m_xScratchIter.get());
-    m_xTreeView->set_image(*m_xScratchIter, sFolderImage);
+    m_xTreeView->set_image(*m_xScratchIter, RID_BMP_FOLDER);
     m_xTreeView->set_cursor(*m_xScratchIter);
 }
 
diff --git a/sd/source/ui/dlg/sdtreelb.cxx b/sd/source/ui/dlg/sdtreelb.cxx
index 38772de43da2..7bb5f06e5c6f 100644
--- a/sd/source/ui/dlg/sdtreelb.cxx
+++ b/sd/source/ui/dlg/sdtreelb.cxx
@@ -1126,12 +1126,10 @@ void SdPageObjsTLV::Fill( const SdDrawDocument* pInDoc, SfxMedium* pInMedium,
     m_pMedium = pInMedium;
     m_aDocName = rDocName;
 
-    OUString sImgDoc(BMP_DOC_OPEN);
-
     OUString sId(OUString::number(1));
     // insert document name
     m_xTreeView->insert(nullptr, -1, &m_aDocName, &sId, nullptr, nullptr, true, m_xScratchIter.get());
-    m_xTreeView->set_image(*m_xScratchIter, sImgDoc);
+    m_xTreeView->set_image(*m_xScratchIter, BMP_DOC_OPEN);
 }
 
 /**
diff --git a/sw/source/ui/fldui/changedb.cxx b/sw/source/ui/fldui/changedb.cxx
index 379e0645906d..1200a8554061 100644
--- a/sw/source/ui/fldui/changedb.cxx
+++ b/sw/source/ui/fldui/changedb.cxx
@@ -112,7 +112,6 @@ std::unique_ptr<weld::TreeIter> SwChangeDBDlg::Insert(const OUString& rDBName)
     OUString sUserData = rDBName.getToken(0, DB_DELIM, nIdx);
     sal_Int32 nCommandType = sUserData.toInt32();
 
-    OUString aDBImg(RID_BMP_DB);
     OUString rToInsert = nCommandType ? OUStringLiteral(RID_BMP_DBQUERY) : OUStringLiteral(RID_BMP_DBTABLE);
 
     std::unique_ptr<weld::TreeIter> xIter(m_xUsedDBTLB->make_iterator());
@@ -144,7 +143,7 @@ std::unique_ptr<weld::TreeIter> SwChangeDBDlg::Insert(const OUString& rDBName)
 
     m_xUsedDBTLB->insert(nullptr, -1, &sDBName, nullptr, nullptr, nullptr,
                          false, xIter.get());
-    m_xUsedDBTLB->set_image(*xIter, aDBImg);
+    m_xUsedDBTLB->set_image(*xIter, RID_BMP_DB);
     m_xUsedDBTLB->insert(xIter.get(), -1, &sTableName, &sUserData, nullptr, nullptr,
                          false, xIter.get());
     m_xUsedDBTLB->set_image(*xIter, rToInsert);
diff --git a/sw/source/uibase/dbui/dbtree.cxx b/sw/source/uibase/dbui/dbtree.cxx
index cdebbaaed4e1..480ced08f784 100644
--- a/sw/source/uibase/dbui/dbtree.cxx
+++ b/sw/source/uibase/dbui/dbtree.cxx
@@ -172,9 +172,8 @@ void SwDBTreeList::InitTreeList()
 
 void SwDBTreeList::AddDataSource(const OUString& rSource)
 {
-    OUString aImg(RID_BMP_DB);
     m_xTreeView->insert(nullptr, -1, &rSource, nullptr, nullptr, nullptr, true, m_xScratchIter.get());
-    m_xTreeView->set_image(*m_xScratchIter, aImg);
+    m_xTreeView->set_image(*m_xScratchIter, RID_BMP_DB);
     m_xTreeView->select(*m_xScratchIter);
 }