diff options
author | Noel Grandin <noel.grandin@collabora.co.uk> | 2021-12-07 14:37:56 +0200 |
---|---|---|
committer | Noel Grandin <noel.grandin@collabora.co.uk> | 2021-12-07 17:46:23 +0100 |
commit | 5de20e45e48f7654d288f26f768fabddad133bfd (patch) | |
tree | cce410ce740ae0ca760855a2ddc2ca09d623e633 /compilerplugins | |
parent | 7e8e57a456f2b946631eecefd163cb4ff3a3d603 (diff) |
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>
Diffstat (limited to 'compilerplugins')
-rw-r--r-- | compilerplugins/clang/cow_wrapper.cxx | 65 | ||||
-rw-r--r-- | compilerplugins/clang/test/cow_wrapper.cxx | 11 |
2 files changed, 60 insertions, 16 deletions
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; }; |