loplugin:staticconstfield improvements

And fix
  ScXMLCachedRowAttrAccess::Cache
which was never setting its mnTab field, and hence would never
be hit.

And fix oox::xls::CellBlockBuffer, which was never setting mnCurrRow.

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

2 files changed, 154 insertions, 83 deletions

diff --git a/compilerplugins/clang/staticconstfield.cxx b/compilerplugins/clang/staticconstfield.cxx
index cde2f80babc5..b10953a75617 100644
--- a/compilerplugins/clang/staticconstfield.cxx
+++ b/compilerplugins/clang/staticconstfield.cxx
@@ -11,6 +11,9 @@
 #include "check.hxx"
 #include "compat.hxx"
 #include <iostream>
+#include <unordered_map>
+#include <unordered_set>
+#include <vector>
 
 namespace
 {
@@ -22,22 +25,76 @@ public:
     {
     }
 
-    void run() override { TraverseDecl(compiler.getASTContext().getTranslationUnitDecl()); }
+    void run() override;
 
     bool TraverseConstructorInitializer(CXXCtorInitializer* init);
+    bool TraverseCXXConstructorDecl(CXXConstructorDecl* decl);
+
+private:
+    struct Data
+    {
+        std::vector<CXXCtorInitializer const*> inits;
+        std::string value;
+    };
+    std::unordered_map<FieldDecl const*, Data> m_potentials;
+    std::unordered_set<FieldDecl const*> m_excluded;
+    CXXConstructorDecl* m_currentConstructor = nullptr;
 };
 
+void StaticConstField::run()
+{
+    std::string fn = handler.getMainFileName();
+    loplugin::normalizeDotDotInFilePath(fn);
+
+    // unusual case where a user constructor sets a field to one value, and a copy constructor sets it to a different value
+    if (fn == SRCDIR "/sw/source/core/attr/hints.cxx")
+        return;
+    if (fn == SRCDIR "/oox/source/core/contexthandler2.cxx")
+        return;
+
+    TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+
+    for (auto const& pair : m_potentials)
+    {
+        report(DiagnosticsEngine::Error, "field can be static const", pair.first->getLocation())
+            << pair.first->getSourceRange();
+        for (CXXCtorInitializer const* init : pair.second.inits)
+            if (pair.first->getLocation() != init->getSourceLocation())
+                report(DiagnosticsEngine::Note, "init here", init->getSourceLocation())
+                    << init->getSourceRange();
+    }
+}
+
+bool StaticConstField::TraverseCXXConstructorDecl(CXXConstructorDecl* decl)
+{
+    auto prev = m_currentConstructor;
+    m_currentConstructor = decl;
+    bool ret = FilteringPlugin::TraverseCXXConstructorDecl(decl);
+    m_currentConstructor = prev;
+    return ret;
+}
+
 bool StaticConstField::TraverseConstructorInitializer(CXXCtorInitializer* init)
 {
     if (!init->getSourceLocation().isValid() || ignoreLocation(init->getSourceLocation()))
         return true;
     if (!init->getMember())
         return true;
+    if (!init->getInit())
+        return true;
+    if (!m_currentConstructor || m_currentConstructor->isCopyOrMoveConstructor())
+        return true;
+    if (!m_currentConstructor->getParent()->isCompleteDefinition())
+        return true;
+    if (m_excluded.find(init->getMember()) != m_excluded.end())
+        return true;
     auto type = init->getMember()->getType();
     auto tc = loplugin::TypeCheck(type);
-    bool found = false;
     if (!tc.Const())
         return true;
+
+    bool found = false;
+    std::string value;
     if (tc.Const().Class("OUString").Namespace("rtl").GlobalNamespace()
         || tc.Const().Class("OString").Namespace("rtl").GlobalNamespace())
     {
@@ -45,61 +102,66 @@ bool StaticConstField::TraverseConstructorInitializer(CXXCtorInitializer* init)
         {
             if (constructExpr->getNumArgs() >= 1
                 && isa<clang::StringLiteral>(constructExpr->getArg(0)))
+            {
+                value = dyn_cast<clang::StringLiteral>(constructExpr->getArg(0))->getString();
                 found = true;
+            }
         }
     }
-    else if (type->isIntegerType())
-    {
-        if (isa<IntegerLiteral>(init->getInit()->IgnoreParenImpCasts()))
-            found = true;
-        // isIntegerType includes bool
-        else if (isa<CXXBoolLiteralExpr>(init->getInit()->IgnoreParenImpCasts()))
-            found = true;
-    }
+#if CLANG_VERSION >= 50000
     else if (type->isFloatingType())
     {
-        if (isa<FloatingLiteral>(init->getInit()->IgnoreParenImpCasts()))
-            found = true;
-    }
-    else if (type->isEnumeralType())
-    {
-        if (auto declRefExpr = dyn_cast<DeclRefExpr>(init->getInit()->IgnoreParenImpCasts()))
+        APFloat x1(0.0f);
+        if (init->getInit()->EvaluateAsFloat(x1, compiler.getASTContext()))
         {
-            if (isa<EnumConstantDecl>(declRefExpr->getDecl()))
-                found = true;
+            std::string s;
+            llvm::raw_string_ostream os(s);
+            x1.print(os);
+            value = os.str();
+            found = true;
         }
     }
-
-    // If we find more than one non-copy-move constructor, we can't say for sure if a member can be static
-    // because it could be initialised differently in each constructor.
-    if (auto cxxRecordDecl = dyn_cast<CXXRecordDecl>(init->getMember()->getParent()))
+#endif
+    // ignore this, it seems to trigger an infinite recursion
+    else if (isa<UnaryExprOrTypeTraitExpr>(init->getInit()))
+        ;
+    // ignore this, calling EvaluateAsInt on it will crash clang
+    else if (init->getInit()->isValueDependent())
+        ;
+    else
     {
-        int cnt = 0;
-        for (auto it = cxxRecordDecl->ctor_begin(); it != cxxRecordDecl->ctor_end(); ++it)
+        APSInt x1;
+        if (init->getInit()->EvaluateAsInt(x1, compiler.getASTContext()))
         {
-            if (!it->isCopyOrMoveConstructor())
-                cnt++;
+            value = x1.toString(10);
+            found = true;
         }
-        if (cnt > 1)
-            return true;
     }
 
     if (!found)
+    {
+        m_potentials.erase(init->getMember());
+        m_excluded.insert(init->getMember());
         return true;
+    }
 
-    std::string fn = handler.getMainFileName();
-    loplugin::normalizeDotDotInFilePath(fn);
-
-    // unusual case where a user constructor sets a field to one value, and a copy constructor sets it to a different value
-    if (fn == SRCDIR "/sw/source/core/attr/hints.cxx")
-        return true;
-    if (fn == SRCDIR "/oox/source/core/contexthandler2.cxx")
-        return true;
-
-    report(DiagnosticsEngine::Warning, "field can be static const", init->getSourceLocation())
-        << init->getSourceRange();
-    report(DiagnosticsEngine::Note, "field here", init->getMember()->getLocation())
-        << init->getMember()->getSourceRange();
+    auto findIt = m_potentials.find(init->getMember());
+    if (findIt != m_potentials.end())
+    {
+        if (findIt->second.value != value)
+        {
+            m_potentials.erase(findIt);
+            m_excluded.insert(init->getMember());
+        }
+        else
+            findIt->second.inits.push_back(init);
+    }
+    else
+    {
+        Data& data = m_potentials[init->getMember()];
+        data.inits.push_back(init);
+        data.value = value;
+    }
 
     return true;
 }
diff --git a/compilerplugins/clang/test/staticconstfield.cxx b/compilerplugins/clang/test/staticconstfield.cxx
index 03708fcaa9fd..ab0a24fa734a 100644
--- a/compilerplugins/clang/test/staticconstfield.cxx
+++ b/compilerplugins/clang/test/staticconstfield.cxx
@@ -7,15 +7,18 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 
+#include <config_clang.h>
 #include <rtl/ustring.hxx>
 #include <rtl/string.hxx>
+#include <vector>
 
 class Class1
 {
-    OUString const m_field1; // expected-note {{field here [loplugin:staticconstfield]}}
+    OUString const
+        m_field1; // expected-error {{field can be static const [loplugin:staticconstfield]}}
     Class1()
         : m_field1("xxxx")
-    // expected-error@-1 {{field can be static const [loplugin:staticconstfield]}}
+    // expected-note@-1 {{init here [loplugin:staticconstfield]}}
     {
         (void)m_field1;
     }
@@ -23,52 +26,58 @@ class Class1
 
 class Class2
 {
-    OString const m_field1; // expected-note {{field here [loplugin:staticconstfield]}}
+    OString const
+        m_field2; // expected-error {{field can be static const [loplugin:staticconstfield]}}
     Class2()
-        : m_field1("xxxx")
-    // expected-error@-1 {{field can be static const [loplugin:staticconstfield]}}
+        : m_field2("yyyy")
+    // expected-note@-1 {{init here [loplugin:staticconstfield]}}
     {
-        (void)m_field1;
+        (void)m_field2;
     }
 };
 
 // no warning expected
 class Class4
 {
-    OUString m_field1;
+    OUString m_field3;
     Class4()
-        : m_field1("xxxx")
+        : m_field3("zzzz")
     {
-        (void)m_field1;
+        (void)m_field3;
     }
 };
 
+#if CLANG_VERSION >= 50000 // Expr::EvaluateAsFloat
 class Class5
 {
     enum class Enum
     {
         ONE
     };
-    float const m_field1; // expected-note {{field here [loplugin:staticconstfield]}}
-    int const m_field2; // expected-note {{field here [loplugin:staticconstfield]}}
-    bool const m_field3; // expected-note {{field here [loplugin:staticconstfield]}}
-    Enum const m_field4; // expected-note {{field here [loplugin:staticconstfield]}}
+    float const
+        m_fielda1; // expected-error {{field can be static const [loplugin:staticconstfield]}}
+    int const m_fielda2; // expected-error {{field can be static const [loplugin:staticconstfield]}}
+    bool const
+        m_fielda3; // expected-error {{field can be static const [loplugin:staticconstfield]}}
+    Enum const
+        m_fielda4; // expected-error {{field can be static const [loplugin:staticconstfield]}}
     Class5()
-        : m_field1(1.0)
-        // expected-error@-1 {{field can be static const [loplugin:staticconstfield]}}
-        , m_field2(1)
-        // expected-error@-1 {{field can be static const [loplugin:staticconstfield]}}
-        , m_field3(true)
-        // expected-error@-1 {{field can be static const [loplugin:staticconstfield]}}
-        , m_field4(Enum::ONE)
-    // expected-error@-1 {{field can be static const [loplugin:staticconstfield]}}
+        : m_fielda1(1.0)
+        // expected-note@-1 {{init here [loplugin:staticconstfield]}}
+        , m_fielda2(1)
+        // expected-note@-1 {{init here [loplugin:staticconstfield]}}
+        , m_fielda3(true)
+        // expected-note@-1 {{init here [loplugin:staticconstfield]}}
+        , m_fielda4(Enum::ONE)
+    // expected-note@-1 {{init here [loplugin:staticconstfield]}}
     {
-        (void)m_field1;
-        (void)m_field2;
-        (void)m_field3;
-        (void)m_field4;
+        (void)m_fielda1;
+        (void)m_fielda2;
+        (void)m_fielda3;
+        (void)m_fielda4;
     }
 };
+#endif
 
 // no warning expected
 class Class6
@@ -77,36 +86,36 @@ class Class6
     {
         ONE
     };
-    float m_field1;
-    int m_field2;
-    bool m_field3;
-    Enum m_field4;
+    float m_fieldb1;
+    int m_fieldb2;
+    bool m_fieldb3;
+    Enum m_fieldb4;
     Class6()
-        : m_field1(1.0)
-        , m_field2(1)
-        , m_field3(true)
-        , m_field4(Enum::ONE)
+        : m_fieldb1(1.0)
+        , m_fieldb2(1)
+        , m_fieldb3(true)
+        , m_fieldb4(Enum::ONE)
     {
-        (void)m_field1;
-        (void)m_field2;
-        (void)m_field3;
-        (void)m_field4;
+        (void)m_fieldb1;
+        (void)m_fieldb2;
+        (void)m_fieldb3;
+        (void)m_fieldb4;
     }
 };
 
 // no warning expected, checking for assigning to const field from multiple constructors
 class Class7
 {
-    bool const m_field1;
+    bool const m_field7;
     Class7()
-        : m_field1(true)
+        : m_field7(true)
     {
-        (void)m_field1;
+        (void)m_field7;
     }
     Class7(bool b)
-        : m_field1(b)
+        : m_field7(b)
     {
-        (void)m_field1;
+        (void)m_field7;
     }
 };