Re: [PATCH] D13099: [Analyzer] Don’t invalidate CXXThis when conservatively evaluating const methods (PR 21606)

[Sean Eveson via cfe-commits]

seaneveson added a comment.

Thank you for looking at the patch and all your comments.

In http://reviews.llvm.org/D13099#252492, @xazax.hun wrote:

> One more note. Do we want to support const_cast for this? A possible way to do that is to invalidate this, when a const cast appears in the body of the function. (However the body might not be available. It is only my opinion, but I would be ok to accept this patch without const cast support, but it is something that we might want to document somewhere as a caveat.


IMO it is reasonable to assume that a const method wont modify non-mutable members. While it is possible to use casts or other pointers to make changes, I'm not sure it is worth checking for these cases.


================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:409
@@ +408,3 @@
+  if (const CXXMethodDecl *D = cast_or_null<CXXMethodDecl>(getDecl())) {
+    if(D->getCanonicalDecl()->isConst()) {
+      // Check if the containing class/struct has mutable members
----------------
xazax.hun wrote:
> Do we need to get the cannonical decl here? When one declaration is const, all of them supposed to be const.
I was getting a strange case where the PR21606 test didn't work without getCanonical, but when testing it now it seems to be fine. Perhaps something was fixed elsewhere? Regardless I'll remove the call. Thanks.

================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:412
@@ +411,3 @@
+      const MemRegion *ThisRegion = getCXXThisVal().getAsRegion();
+      if (ThisRegion) {
+        MemRegionManager *MemMgr = ThisRegion->getMemRegionManager();
----------------
xazax.hun wrote:
> Is it possible to fail to get ThisRegion? Should this be an assert?
I put the test in because getCXXThisVal can return an UnknownVal, but on closer inspection I don't think this will ever happen for a CXXMemberCall, so I will change this to an assert. Thanks.

================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:415
@@ +414,3 @@
+        const CXXRecordDecl *Parent = D->getParent();
+        for (const auto *I : Parent->fields()) {
+          if (I->isMutable()) {
----------------
xazax.hun wrote:
> What about the mutable fields of base classes? Are they covered here?
Good point, I don't think they are so I'll work on that.


http://reviews.llvm.org/D13099