loplugin:unusedfields make it a little smarter

around dealing with operator[] on map data-types

Change-Id: Idd6654948ae2d03d634fcf30a8d98530a78ab4ca
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/153740
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>

2 files changed, 76 insertions, 12 deletions

diff --git a/compilerplugins/clang/test/unusedfields.cxx b/compilerplugins/clang/test/unusedfields.cxx
index b545f4b1a89f..c936460bbea5 100644
--- a/compilerplugins/clang/test/unusedfields.cxx
+++ b/compilerplugins/clang/test/unusedfields.cxx
@@ -11,6 +11,7 @@
 // expected-no-diagnostics
 #else
 
+#include <map>
 #include <memory>
 #include <vector>
 #include <ostream>
@@ -377,6 +378,49 @@ namespace TouchFromOutsideAnalysis1
     };
 };
 
+namespace WriteOnlyAnalysis4
+{
+    struct ImplTabCtrlData
+    // expected-error@-1 {{read maLayoutLineToPageId [loplugin:unusedfields]}}
+    // expected-error@-2 {{outside maLayoutLineToPageId [loplugin:unusedfields]}}
+    {
+        std::map< int, int > maLayoutLineToPageId;
+    };
+    class TabControl
+    // expected-error@-1 {{read mpTabCtrlData [loplugin:unusedfields]}}
+    // expected-error@-2 {{outside-constructor mpTabCtrlData [loplugin:unusedfields]}}
+    {
+        std::unique_ptr<ImplTabCtrlData> mpTabCtrlData;
+
+        void foo(int nLine, int& rPageId)
+        {
+            rPageId = mpTabCtrlData->maLayoutLineToPageId[ nLine ];
+        }
+    };
+}
+
+namespace WriteOnlyAnalysis5
+{
+    struct ImplTabCtrlData
+    // expected-error@-1 {{read maLayoutLineToPageId [loplugin:unusedfields]}}
+    // expected-error@-2 {{write maLayoutLineToPageId [loplugin:unusedfields]}}
+    // expected-error@-3 {{outside maLayoutLineToPageId [loplugin:unusedfields]}}
+    {
+        std::map< int, int > maLayoutLineToPageId;
+    };
+    class TabControl
+    // expected-error@-1 {{read mpTabCtrlData [loplugin:unusedfields]}}
+    // expected-error@-2 {{outside-constructor mpTabCtrlData [loplugin:unusedfields]}}
+    {
+        std::unique_ptr<ImplTabCtrlData> mpTabCtrlData;
+
+        void foo(int nLine, int nPageId)
+        {
+            mpTabCtrlData->maLayoutLineToPageId[ nLine ] = nPageId;
+        }
+    };
+}
+
 #endif
 
 /* 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 68588cd7c694..ca5eb3eb00b3 100644
--- a/compilerplugins/clang/unusedfields.cxx
+++ b/compilerplugins/clang/unusedfields.cxx
@@ -168,6 +168,7 @@ private:
     void checkIfWrittenTo(const FieldDecl* fieldDecl, const Expr* memberExpr);
     bool isSomeKindOfZero(const Expr* arg);
     bool checkForWriteWhenUsingCollectionType(const CXXMethodDecl * calleeMethodDecl);
+    bool checkForUsingMap(const CXXMethodDecl * calleeMethodDecl);
     bool IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt * child, CallerWrapper callExpr,
                                         CalleeWrapper calleeFunctionDecl);
     compat::optional<CalleeWrapper> getCallee(CallExpr const *);
@@ -831,6 +832,7 @@ void UnusedFields::checkIfWrittenTo(const FieldDecl* fieldDecl, const Expr* memb
         }
         else if (auto operatorCallExpr = dyn_cast<CXXOperatorCallExpr>(parent))
         {
+            bool walk = false;
             auto callee = getCallee(operatorCallExpr);
             if (callee)
             {
@@ -839,16 +841,24 @@ void UnusedFields::checkIfWrittenTo(const FieldDecl* fieldDecl, const Expr* memb
                 if (calleeMethodDecl && operatorCallExpr->getArg(0) == child)
                 {
                     if (!calleeMethodDecl->isConst())
-                        bPotentiallyWrittenTo = checkForWriteWhenUsingCollectionType(calleeMethodDecl);
+                    {
+                        // If we are accessing a map entry, we want to keep walking up to determine
+                        // if it is written to.
+                        if (checkForUsingMap(calleeMethodDecl))
+                            walk = true;
+                        else
+                            bPotentiallyWrittenTo = checkForWriteWhenUsingCollectionType(calleeMethodDecl);
+                    }
                 }
                 else if (IsPassedByNonConst(fieldDecl, child, operatorCallExpr, *callee))
-                {
                     bPotentiallyWrittenTo = true;
-                }
             }
             else
                 bPotentiallyWrittenTo = true; // conservative, could improve
-            break;
+            if (walk)
+                walkUp();
+            else
+                break;
         }
         else if (auto cxxMemberCallExpr = dyn_cast<CXXMemberCallExpr>(parent))
         {
@@ -980,20 +990,20 @@ bool UnusedFields::checkForWriteWhenUsingCollectionType(const CXXMethodDecl * ca
 {
     auto const tc = loplugin::TypeCheck(calleeMethodDecl->getParent());
     bool listLike = false, setLike = false, mapLike = false, cssSequence = false;
-    if (tc.Class("deque").StdNamespace()
-        || tc.Class("list").StdNamespace()
-        || tc.Class("queue").StdNamespace()
-        || tc.Class("vector").StdNamespace())
+    // Noting that I am deliberately not calling StdNamespace() on these checks, the loplugin::TypeCheck
+    // code seems to be unreliable when dealing with ClassTemplateSpecializationDecl.
+    if (tc.Class("deque")
+        || tc.Class("list")
+        || tc.Class("queue")
+        || tc.Class("vector"))
     {
         listLike = true;
     }
-    else if (tc.Class("set").StdNamespace()
-        || tc.Class("unordered_set").StdNamespace())
+    else if (tc.Class("set") || tc.Class("unordered_set"))
     {
         setLike = true;
     }
-    else if (tc.Class("map").StdNamespace()
-        || tc.Class("unordered_map").StdNamespace())
+    else if (tc.Class("map") || tc.Class("unordered_map"))
     {
         mapLike = true;
     }
@@ -1040,6 +1050,16 @@ bool UnusedFields::checkForWriteWhenUsingCollectionType(const CXXMethodDecl * ca
     return true;
 }
 
+bool UnusedFields::checkForUsingMap(const CXXMethodDecl * calleeMethodDecl)
+{
+    auto const tc = loplugin::TypeCheck(calleeMethodDecl->getParent());
+    if (!(tc.Class("map") || tc.Class("unordered_map")))
+        return false;
+    if (!calleeMethodDecl->isOverloadedOperator())
+        return false;
+    return calleeMethodDecl->getOverloadedOperator() == OO_Subscript;
+}
+
 bool UnusedFields::IsPassedByNonConst(const FieldDecl* fieldDecl, const Stmt * child, CallerWrapper callExpr,
                                          CalleeWrapper calleeFunctionDecl)
 {