From 5de20e45e48f7654d288f26f768fabddad133bfd Mon Sep 17 00:00:00 2001
From: Noel Grandin <noel.grandin@collabora.co.uk>
Date: Tue, 7 Dec 2021 14:37:56 +0200
Subject: improve loplugin:cow_wrapper

to find my previous attempt at this, which only obscured the problem

<noelgrandin> I'm such an idiot
<noelgrandin> I changed a whole bunch of code to avoid calling const
methods on a non-const object
<noelgrandin> from p->foo() to std::as_const(*p).foo()
<noelgrandin> can you spot the mistake?
<bubli> Is this a job interview question? :D
<vmiklos> noelgrandin: you did the opposite, now you always call const
member functions, while you wanted to always call non-const member
functions?
<noelgrandin> more like a "why didn't the smart people on this channel
tell me I was an idiot" :-)
<noelgrandin> in this case, we have o3tl::cow_wrapper, which overrides
operator* and operator->
<vmiklos> ah, and by the time you would add/remove the const,
cow_wrapper already did the expensive task of copying based on
const/non-const
<noelgrandin> exactly
<thorsten> heh

Change-Id: I5366e6a87c414b862668b61e6adfbccfdd9d3b04
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/126473
Tested-by: Jenkins
Reviewed-by: Noel Grandin <noel.grandin@collabora.co.uk>
---
 compilerplugins/clang/cow_wrapper.cxx      | 65 ++++++++++++++++++++++--------
 compilerplugins/clang/test/cow_wrapper.cxx | 11 +++++
 2 files changed, 60 insertions(+), 16 deletions(-)

(limited to 'compilerplugins')

diff --git a/compilerplugins/clang/cow_wrapper.cxx b/compilerplugins/clang/cow_wrapper.cxx
index 4c04674f65c1..d5d1b47b3a43 100644
--- a/compilerplugins/clang/cow_wrapper.cxx
+++ b/compilerplugins/clang/cow_wrapper.cxx
@@ -53,22 +53,55 @@ bool Cow_Wrapper::VisitCXXMemberCallExpr(const CXXMemberCallExpr* memberCallExpr
     if (!methodDecl || !methodDecl->isConst())
         return true;
 
-    auto operatorCallExpr = dyn_cast<CXXOperatorCallExpr>(
-        compat::IgnoreImplicit(memberCallExpr->getImplicitObjectArgument()));
-    if (!operatorCallExpr)
-        return true;
-    if (operatorCallExpr->getOperator() != OO_Arrow)
-        return true;
-    auto arrowMethodDecl = dyn_cast_or_null<CXXMethodDecl>(operatorCallExpr->getDirectCallee());
-    if (!arrowMethodDecl)
-        return true;
-    if (arrowMethodDecl->isConst())
-        return true;
-    auto dc = loplugin::DeclCheck(arrowMethodDecl->getParent())
-                  .Class("cow_wrapper")
-                  .Namespace("o3tl")
-                  .GlobalNamespace();
-    if (!dc)
+    auto expr = compat::IgnoreImplicit(memberCallExpr->getImplicitObjectArgument())->IgnoreParens();
+    auto operatorCallExpr = dyn_cast<CXXOperatorCallExpr>(expr);
+
+    if (operatorCallExpr && operatorCallExpr->getOperator() == OO_Arrow)
+    {
+        auto arrowMethodDecl = dyn_cast_or_null<CXXMethodDecl>(operatorCallExpr->getDirectCallee());
+        if (!arrowMethodDecl)
+            return true;
+        if (arrowMethodDecl->isConst())
+            return true;
+        auto dc = loplugin::DeclCheck(arrowMethodDecl->getParent())
+                      .Class("cow_wrapper")
+                      .Namespace("o3tl")
+                      .GlobalNamespace();
+        if (!dc)
+            return true;
+    }
+    else if (operatorCallExpr)
+    {
+        auto methodDecl2 = dyn_cast_or_null<CXXMethodDecl>(operatorCallExpr->getDirectCallee());
+        if (!methodDecl2)
+            return true;
+        auto dc = loplugin::DeclCheck(methodDecl2->getParent())
+                      .Class("cow_wrapper")
+                      .Namespace("o3tl")
+                      .GlobalNamespace();
+        if (!dc)
+            return true;
+    }
+    else if (auto callExpr = dyn_cast<CallExpr>(expr))
+    {
+        if (!isa<ImplicitCastExpr>(callExpr->getCallee())) // std::as_const shows up as this
+            return true;
+        if (callExpr->getNumArgs() < 1)
+            return true;
+        auto arg0 = dyn_cast<CXXOperatorCallExpr>(callExpr->getArg(0));
+        if (!arg0)
+            return true;
+        auto starMethodDecl = dyn_cast_or_null<CXXMethodDecl>(arg0->getDirectCallee());
+        if (!starMethodDecl)
+            return true;
+        auto dc = loplugin::DeclCheck(starMethodDecl->getParent())
+                      .Class("cow_wrapper")
+                      .Namespace("o3tl")
+                      .GlobalNamespace();
+        if (!dc)
+            return true;
+    }
+    else
         return true;
 
     report(DiagnosticsEngine::Warning,
diff --git a/compilerplugins/clang/test/cow_wrapper.cxx b/compilerplugins/clang/test/cow_wrapper.cxx
index 705a4052ef66..5c95f87f04d9 100644
--- a/compilerplugins/clang/test/cow_wrapper.cxx
+++ b/compilerplugins/clang/test/cow_wrapper.cxx
@@ -9,6 +9,7 @@
 
 #include "config_clang.h"
 #include "o3tl/cow_wrapper.hxx"
+#include <utility>
 
 struct ImplBitmapPalette
 {
@@ -27,6 +28,16 @@ struct BitmapPalette
         // no error expected
         mpImpl->foo();
     }
+    void foo3()
+    {
+        // expected-error@+1 {{calling const method on o3tl::cow_wrapper impl class via non-const pointer, rather use std::as_const to prevent triggering an unnecessary copy [loplugin:cow_wrapper]}}
+        (*mpImpl).foo();
+    }
+    void foo4()
+    {
+        // expected-error@+1 {{calling const method on o3tl::cow_wrapper impl class via non-const pointer, rather use std::as_const to prevent triggering an unnecessary copy [loplugin:cow_wrapper]}}
+        std::as_const(*mpImpl).foo();
+    }
     o3tl::cow_wrapper<ImplBitmapPalette> mpImpl;
 };
 
-- 
cgit