[PATCH] D25475: [analyzer] Add a new SVal to support pointer-to-member operations.

[Artem Dergachev via cfe-commits]

NoQ added a comment.

Yay, thanks for posting this! :)

I've got a bit of concern for some assert-suppressions.



================
Comment at: lib/StaticAnalyzer/Core/ExprEngine.cpp:2314
 {
+  // Return to fulfil assert condition
+  if (location.getAs<nonloc::PointerToMember>())
----------------
Hmm. Why would anybody try to load anything from a plain pointer-to-member, rather than from a pointer-to-member-applied-to-an-object (which would no longer be represented by a `PointerToMember` value)? I suspect there's something wrong above the stack (or one of the sub-expression `SVal`s is incorrect), because otherwise i think that making `PointerToMember` a NonLoc is correct - we cannot store things in it or load things from it.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineC.cpp:465
+               "UnaryOperator as Cast's child was expected");
+        if (const UnaryOperator *UO = dyn_cast<UnaryOperator>(UOExpr)) {
+          const Expr *DREExpr = UO->getSubExpr()->IgnoreParenCasts();
----------------
`cast<>()`? It seems that all dynamic casts here are asserted to succeed.


================
Comment at: lib/StaticAnalyzer/Core/ExprEngineCXX.cpp:69
+    // Add check to fulfil assert condition
+    if (!V.getAs<nonloc::PointerToMember>())
+      assert(V.isUnknown());
----------------
Same concern: Why are we copying a `NonLoc`?


================
Comment at: test/Analysis/pointer-to-member.cpp:79
   // FIXME: Should emit a null dereference.
   return obj.*member; // no-warning
 }
----------------
In fact, maybe dereferencing a null pointer-to-member should produce an `UndefinedVal`, which could be later caught by `core.uninitialized.UndefReturn`. I wonder why doesn't this happen.


https://reviews.llvm.org/D25475