loplugin:staticconstfield improvements

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

2 files changed, 124 insertions, 13 deletions

diff --git a/compilerplugins/clang/staticconstfield.cxx b/compilerplugins/clang/staticconstfield.cxx
index 7276d7e05c49..bfc32a8c059f 100644
--- a/compilerplugins/clang/staticconstfield.cxx
+++ b/compilerplugins/clang/staticconstfield.cxx
@@ -33,22 +33,66 @@ bool StaticConstField::TraverseConstructorInitializer(CXXCtorInitializer* init)
         return true;
     if (!init->getMember())
         return true;
-    auto tc = loplugin::TypeCheck(init->getMember()->getType());
-    if (!tc.Const().Class("OUString").Namespace("rtl").GlobalNamespace()
-        && !tc.Const().Class("OString").Namespace("rtl").GlobalNamespace())
+    auto type = init->getMember()->getType();
+    auto tc = loplugin::TypeCheck(type);
+    bool found = false;
+    if (!tc.Const())
         return true;
-    if (auto constructExpr = dyn_cast<CXXConstructExpr>(init->getInit()))
+    if (tc.Const().Class("OUString").Namespace("rtl").GlobalNamespace()
+        || tc.Const().Class("OString").Namespace("rtl").GlobalNamespace())
     {
-        if (constructExpr->getNumArgs() >= 1 && isa<clang::StringLiteral>(constructExpr->getArg(0)))
+        if (auto constructExpr = dyn_cast<CXXConstructExpr>(init->getInit()))
         {
-            report(DiagnosticsEngine::Warning, "string field can be static const",
-                   init->getSourceLocation())
-                << init->getSourceRange();
-            report(DiagnosticsEngine::Note, "field here", init->getMember()->getLocation())
-                << init->getMember()->getSourceRange();
+            if (constructExpr->getNumArgs() >= 1
+                && isa<clang::StringLiteral>(constructExpr->getArg(0)))
+                found = true;
         }
     }
-    return RecursiveASTVisitor::TraverseConstructorInitializer(init);
+    else if (type->isIntegerType())
+    {
+        if (isa<IntegerLiteral>(init->getInit()->IgnoreParenImpCasts()))
+            found = true;
+        // isIntegerType includes bool
+        else if (isa<CXXBoolLiteralExpr>(init->getInit()->IgnoreParenImpCasts()))
+            found = true;
+    }
+    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()))
+        {
+            if (isa<EnumConstantDecl>(declRefExpr->getDecl()))
+                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()))
+    {
+        int cnt = 0;
+        for (auto it = cxxRecordDecl->ctor_begin(); it != cxxRecordDecl->ctor_end(); ++it)
+        {
+            if (!it->isCopyOrMoveConstructor())
+                cnt++;
+        }
+        if (cnt > 1)
+            return true;
+    }
+
+    if (found)
+    {
+        report(DiagnosticsEngine::Warning, "field can be static const", init->getSourceLocation())
+            << init->getSourceRange();
+        report(DiagnosticsEngine::Note, "field here", init->getMember()->getLocation())
+            << init->getMember()->getSourceRange();
+    }
+
+    return true;
 }
 
 loplugin::Plugin::Registration<StaticConstField> X("staticconstfield", true);
diff --git a/compilerplugins/clang/test/staticconstfield.cxx b/compilerplugins/clang/test/staticconstfield.cxx
index 49b326f7d76e..03708fcaa9fd 100644
--- a/compilerplugins/clang/test/staticconstfield.cxx
+++ b/compilerplugins/clang/test/staticconstfield.cxx
@@ -15,7 +15,7 @@ class Class1
     OUString const m_field1; // expected-note {{field here [loplugin:staticconstfield]}}
     Class1()
         : m_field1("xxxx")
-    // expected-error@-1 {{string field can be static const [loplugin:staticconstfield]}}
+    // expected-error@-1 {{field can be static const [loplugin:staticconstfield]}}
     {
         (void)m_field1;
     }
@@ -26,7 +26,7 @@ class Class2
     OString const m_field1; // expected-note {{field here [loplugin:staticconstfield]}}
     Class2()
         : m_field1("xxxx")
-    // expected-error@-1 {{string field can be static const [loplugin:staticconstfield]}}
+    // expected-error@-1 {{field can be static const [loplugin:staticconstfield]}}
     {
         (void)m_field1;
     }
@@ -43,4 +43,71 @@ class Class4
     }
 };
 
+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]}}
+    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]}}
+    {
+        (void)m_field1;
+        (void)m_field2;
+        (void)m_field3;
+        (void)m_field4;
+    }
+};
+
+// no warning expected
+class Class6
+{
+    enum class Enum
+    {
+        ONE
+    };
+    float m_field1;
+    int m_field2;
+    bool m_field3;
+    Enum m_field4;
+    Class6()
+        : m_field1(1.0)
+        , m_field2(1)
+        , m_field3(true)
+        , m_field4(Enum::ONE)
+    {
+        (void)m_field1;
+        (void)m_field2;
+        (void)m_field3;
+        (void)m_field4;
+    }
+};
+
+// no warning expected, checking for assigning to const field from multiple constructors
+class Class7
+{
+    bool const m_field1;
+    Class7()
+        : m_field1(true)
+    {
+        (void)m_field1;
+    }
+    Class7(bool b)
+        : m_field1(b)
+    {
+        (void)m_field1;
+    }
+};
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */