[PATCH] D26837: [analyzer] Litter the SVal/SymExpr/MemRegion class hierarchy with asserts.

[Anna Zaks via Phabricator via cfe-commits]

zaks.anna added inline comments.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:23
 #include "clang/StaticAnalyzer/Core/PathSensitive/StoreRef.h"
+#include "clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h"
 
----------------
It's a bit sad to see a header added to another header just for the sake of the asserts.. Maybe we could move the constructors into a cpp file...


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/MemRegion.h:737
 ///  either a real region, a NULL pointer, etc.  It essentially is used to
 ///  map the concept of symbolic values into the domain of regions.  Symbolic
 ///  regions do not need to be typed.
----------------
This comment states that SymbolicRegions do not need to be typed and the assert states that it has to be a pointer type.

Also, what about nullPointerType, isMemberPointerType? I am afraid that our test coverage is not very good and might not catch all cases.

I guess isArrayType() and void types are not expected here?



================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h:48
+    // this may become much stricter.
+    return !T.isNull() && !T->isVoidType();
+  }
----------------
This function seems to promise more than it does. Maybe it should be renamed into something like isNotNullNorVoidType?


================
Comment at: lib/StaticAnalyzer/Core/SVals.cpp:32
 
-bool SVal::hasConjuredSymbol() const {
-  if (Optional<nonloc::SymbolVal> SV = getAs<nonloc::SymbolVal>()) {
----------------
This dead code should be removed in a separate patch.


https://reviews.llvm.org/D26837