loplugin:unusedfields fix false positive

When the field in question is read from inside a constructor
initializer.

In the process, create some needed infrastructure in the plugin classes.

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

7 files changed, 102 insertions, 22 deletions

diff --git a/compilerplugins/clang/plugin.cxx b/compilerplugins/clang/plugin.cxx
index 3e1111c612ef..ea646d29da89 100644
--- a/compilerplugins/clang/plugin.cxx
+++ b/compilerplugins/clang/plugin.cxx
@@ -265,6 +265,11 @@ SourceLocation Plugin::locationAfterToken( SourceLocation location )
     return Lexer::getLocForEndOfToken( location, 0, compiler.getSourceManager(), compiler.getLangOpts());
     }
 
+bool Plugin::isUnitTestMode()
+    {
+    return PluginHandler::isUnitTestMode();
+    }
+
 RewritePlugin::RewritePlugin( const InstantiationData& data )
     : Plugin( data )
     , rewriter( data.rewriter )
diff --git a/compilerplugins/clang/plugin.hxx b/compilerplugins/clang/plugin.hxx
index 413b2ab7cefb..5651b2ff02ca 100644
--- a/compilerplugins/clang/plugin.hxx
+++ b/compilerplugins/clang/plugin.hxx
@@ -78,6 +78,8 @@ class Plugin
         bool isInUnoIncludeFile(const FunctionDecl*) const;
 
         static void normalizeDotDotInFilePath(std::string&);
+
+        static bool isUnitTestMode();
     private:
         static void registerPlugin( Plugin* (*create)( const InstantiationData& ), const char* optionName, bool isPPCallback, bool byDefault );
         template< typename T > static Plugin* createHelper( const InstantiationData& data );
diff --git a/compilerplugins/clang/pluginhandler.cxx b/compilerplugins/clang/pluginhandler.cxx
index ad043e87e58d..7c42d14f3056 100644
--- a/compilerplugins/clang/pluginhandler.cxx
+++ b/compilerplugins/clang/pluginhandler.cxx
@@ -56,13 +56,13 @@ const int MAX_PLUGINS = 100;
 static PluginData plugins[ MAX_PLUGINS ];
 static int pluginCount = 0;
 static bool bPluginObjectsCreated = false;
+static bool unitTestMode = false;
 
 PluginHandler::PluginHandler( CompilerInstance& compiler, const vector< string >& args )
     : compiler( compiler )
     , rewriter( compiler.getSourceManager(), compiler.getLangOpts())
     , scope( "mainfile" )
     , warningsAsErrors( false )
-    , unitTestMode( false )
     {
     set< string > rewriters;
     for( string const & arg : args )
@@ -89,6 +89,11 @@ PluginHandler::~PluginHandler()
             }
     }
 
+bool PluginHandler::isUnitTestMode()
+    {
+    return unitTestMode;
+    }
+
 void PluginHandler::handleOption( const string& option )
     {
     if( option.substr( 0, 6 ) == "scope=" )
@@ -121,10 +126,13 @@ void PluginHandler::createPlugins( set< string > rewriters )
          i < pluginCount;
          ++i )
         {
-        if( rewriters.erase( plugins[i].optionName ) != 0 )
-            plugins[ i ].object = plugins[ i ].create( Plugin::InstantiationData { plugins[ i ].optionName, *this, compiler, &rewriter } );
+        const char* name = plugins[i].optionName;
+        if( rewriters.erase( name ) != 0 )
+            plugins[ i ].object = plugins[ i ].create( Plugin::InstantiationData { name, *this, compiler, &rewriter } );
         else if( plugins[ i ].byDefault )
-            plugins[ i ].object = plugins[ i ].create( Plugin::InstantiationData { plugins[ i ].optionName, *this, compiler, NULL } );
+            plugins[ i ].object = plugins[ i ].create( Plugin::InstantiationData { name, *this, compiler, NULL } );
+        else if( unitTestMode && strcmp(name, "unusedmethodsremove") != 0 && strcmp(name, "unusedfieldsremove") != 0)
+            plugins[ i ].object = plugins[ i ].create( Plugin::InstantiationData { name, *this, compiler, NULL } );
         }
     for( auto r: rewriters )
         report( DiagnosticsEngine::Fatal, "unknown plugin tool %0" ) << r;
diff --git a/compilerplugins/clang/pluginhandler.hxx b/compilerplugins/clang/pluginhandler.hxx
index a2cc136f5751..222cb258e80f 100644
--- a/compilerplugins/clang/pluginhandler.hxx
+++ b/compilerplugins/clang/pluginhandler.hxx
@@ -37,6 +37,7 @@ class PluginHandler
         DiagnosticBuilder report( DiagnosticsEngine::Level level, const char * plugin, StringRef message,
             CompilerInstance& compiler, SourceLocation loc = SourceLocation());
         bool addRemoval( SourceLocation loc );
+        static bool isUnitTestMode();
     private:
         void handleOption( const string& option );
         void createPlugins( set< string > rewriters );
@@ -47,7 +48,6 @@ class PluginHandler
         string scope;
         string warningsOnly;
         bool warningsAsErrors;
-        bool unitTestMode;
     };
 
 /**
diff --git a/compilerplugins/clang/test/unusedfields.cxx b/compilerplugins/clang/test/unusedfields.cxx
new file mode 100644
index 000000000000..f549a9a37673
--- /dev/null
+++ b/compilerplugins/clang/test/unusedfields.cxx
@@ -0,0 +1,28 @@
+/* -*- 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/.
+ */
+
+#include <rtl/ustring.hxx>
+
+struct Foo
+// expected-error@-1 {{read m_foo1 [loplugin:unusedfields]}}
+{
+    int m_foo1;
+};
+
+struct Bar
+// expected-error@-1 {{read m_bar2 [loplugin:unusedfields]}}
+{
+    int m_bar1;
+    int m_bar2 = 1;
+    Bar(Foo const & foo) : m_bar1(foo.m_foo1) {}
+
+    int bar() { return m_bar2; }
+};
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab cinoptions=b1,g0,N-s cinkeys+=0=break: */
diff --git a/compilerplugins/clang/unusedfields.cxx b/compilerplugins/clang/unusedfields.cxx
index e77af78870ab..df64c987f347 100644
--- a/compilerplugins/clang/unusedfields.cxx
+++ b/compilerplugins/clang/unusedfields.cxx
@@ -44,6 +44,7 @@ namespace {
 
 struct MyFieldInfo
 {
+    const RecordDecl* parentRecord;
     std::string parentClass;
     std::string fieldName;
     std::string fieldType;
@@ -71,10 +72,25 @@ class UnusedFields:
 public:
     explicit UnusedFields(InstantiationData const & data): Plugin(data) {}
 
-    virtual void run() override
-    {
-        TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+    virtual void run() override;
+
+    bool shouldVisitTemplateInstantiations () const { return true; }
+    bool shouldVisitImplicitCode() const { return true; }
+
+    bool VisitFieldDecl( const FieldDecl* );
+    bool VisitMemberExpr( const MemberExpr* );
+    bool VisitDeclRefExpr( const DeclRefExpr* );
+private:
+    MyFieldInfo niceName(const FieldDecl*);
+    void checkTouched(const FieldDecl* fieldDecl, const Expr* memberExpr);
+};
 
+void UnusedFields::run()
+{
+    TraverseDecl(compiler.getASTContext().getTranslationUnitDecl());
+
+    if (!isUnitTestMode())
+    {
         // dump all our output in one write call - this is to try and limit IO "crosstalk" between multiple processes
         // writing to the same logfile
         std::string output;
@@ -95,17 +111,19 @@ public:
         myfile << output;
         myfile.close();
     }
+    else
+    {
+        for (const MyFieldInfo & s : readFromSet)
+        {
+            report(
+                DiagnosticsEngine::Warning,
+                "read %0",
+                s.parentRecord->getLocStart())
+                << s.fieldName;
+        }
+    }
+}
 
-    bool shouldVisitTemplateInstantiations () const { return true; }
-    bool shouldVisitImplicitCode() const { return true; }
-
-    bool VisitFieldDecl( const FieldDecl* );
-    bool VisitMemberExpr( const MemberExpr* );
-    bool VisitDeclRefExpr( const DeclRefExpr* );
-private:
-    MyFieldInfo niceName(const FieldDecl*);
-    void checkTouched(const FieldDecl* fieldDecl, const Expr* memberExpr);
-};
 
 MyFieldInfo UnusedFields::niceName(const FieldDecl* fieldDecl)
 {
@@ -117,10 +135,14 @@ MyFieldInfo UnusedFields::niceName(const FieldDecl* fieldDecl)
     {
         if (cxxRecordDecl->getTemplateInstantiationPattern())
             cxxRecordDecl = cxxRecordDecl->getTemplateInstantiationPattern();
+        aInfo.parentRecord = cxxRecordDecl;
         aInfo.parentClass = cxxRecordDecl->getQualifiedNameAsString();
     }
     else
+    {
+        aInfo.parentRecord = recordDecl;
         aInfo.parentClass = recordDecl->getQualifiedNameAsString();
+    }
 
     aInfo.fieldName = fieldDecl->getNameAsString();
     // sometimes the name (if it's anonymous thing) contains the full path of the build folder, which we don't need
@@ -217,20 +239,33 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
 
   // for the write-only analysis
 
+    auto parentsRange = compiler.getASTContext().getParents(*memberExpr);
     const Stmt* child = memberExpr;
-    const Stmt* parent = parentStmt(memberExpr);
+    const Stmt* parent = parentsRange.begin() == parentsRange.end() ? nullptr : parentsRange.begin()->get<Stmt>();
     // walk up the tree until we find something interesting
     bool bPotentiallyReadFrom = false;
     bool bDump = false;
     do {
         if (!parent) {
-            return true;
+            // check if we're inside a CXXCtorInitializer
+            auto parentsRange = compiler.getASTContext().getParents(*child);
+            if ( parentsRange.begin() != parentsRange.end())
+            {
+                const Decl* decl = parentsRange.begin()->get<Decl>();
+                if (decl && isa<CXXConstructorDecl>(decl))
+                    bPotentiallyReadFrom = true;
+            }
+            if (!bPotentiallyReadFrom)
+                return true;
+            else
+                break;
         }
         if (isa<CastExpr>(parent) || isa<MemberExpr>(parent) || isa<ParenExpr>(parent) || isa<ParenListExpr>(parent)
              || isa<ExprWithCleanups>(parent) || isa<UnaryOperator>(parent))
         {
             child = parent;
-            parent = parentStmt(parent);
+            auto parentsRange = compiler.getASTContext().getParents(*parent);
+            parent = parentsRange.begin() == parentsRange.end() ? nullptr : parentsRange.begin()->get<Stmt>();
         }
         else if (isa<CaseStmt>(parent))
         {
@@ -278,7 +313,8 @@ bool UnusedFields::VisitMemberExpr( const MemberExpr* memberExpr )
             // so walk up the tree.
             if (binaryOp->getLHS() == child && op == BO_Assign) {
                 child = parent;
-                parent = parentStmt(parent);
+                auto parentsRange = compiler.getASTContext().getParents(*parent);
+                parent = parentsRange.begin() == parentsRange.end() ? nullptr : parentsRange.begin()->get<Stmt>();
             } else {
                 bPotentiallyReadFrom = true;
                 break;
diff --git a/solenv/CompilerTest_compilerplugins_clang.mk b/solenv/CompilerTest_compilerplugins_clang.mk
index 4e65a684efd4..3de8b08b1cf7 100644
--- a/solenv/CompilerTest_compilerplugins_clang.mk
+++ b/solenv/CompilerTest_compilerplugins_clang.mk
@@ -27,6 +27,7 @@ $(eval $(call gb_CompilerTest_add_exception_objects,compilerplugins_clang, \
     compilerplugins/clang/test/stringconstant \
     compilerplugins/clang/test/unnecessaryoverride-dtor \
     compilerplugins/clang/test/unoany \
+    compilerplugins/clang/test/unusedfields \
     compilerplugins/clang/test/useuniqueptr \
     compilerplugins/clang/test/vclwidgets \
 ))