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

[Gábor Horváth via cfe-commits]

xazax.hun added a comment.

Hi!

Thanks for this patch! I think this would be a great addition! I have some comments inline.


================
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
----------------
Do we need to get the cannonical decl here? When one declaration is const, all of them supposed to be const.

================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:412
@@ +411,3 @@
+      const MemRegion *ThisRegion = getCXXThisVal().getAsRegion();
+      if (ThisRegion) {
+        MemRegionManager *MemMgr = ThisRegion->getMemRegionManager();
----------------
Is it possible to fail to get ThisRegion? Should this be an assert?

================
Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:415
@@ +414,3 @@
+        const CXXRecordDecl *Parent = D->getParent();
+        for (const auto *I : Parent->fields()) {
+          if (I->isMutable()) {
----------------
What about the mutable fields of base classes? Are they covered here?


http://reviews.llvm.org/D13099