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

[Aleksei Sidorin via cfe-commits]

a.sidorin added a comment.

Personally, I like this change because it makes our assumptions clearer. My comments are below.



================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/BasicValueFactory.h:54
+      : store(st), region(r) {
+    assert(r->getValueType()->isRecordType() ||
+           r->getValueType()->isArrayType() ||
----------------
Could you extract `r->getValueType()` to a separate variable?


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:319
 public:
-  SymbolVal(SymbolRef sym) : NonLoc(SymbolValKind, sym) {}
+  SymbolVal() = delete;
+  SymbolVal(SymbolRef sym) : NonLoc(SymbolValKind, sym) { assert(sym); }
----------------
I cannot completely agree with this change. For example, some STL container methods require their type to be default constructible. Yes, it is a very limited usage case but  I don't see strong reason to do it because there also may be some another usage scenarios.


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SVals.h:478
 public:
-  explicit GotoLabel(LabelDecl *Label) : Loc(GotoLabelKind, Label) {}
+  explicit GotoLabel(LabelDecl *Label) : Loc(GotoLabelKind, Label) {
+    assert(Label);
----------------
By the way, why does this ctor accept a non-constant pointer?


================
Comment at: include/clang/StaticAnalyzer/Core/PathSensitive/SymExpr.h:46
+  static bool isValidTypeForSymbol(QualType T) {
+    // FIXME: Depending on wether we choose to deprecate structural symbols,
+    // this may become much stricter.
----------------
s/wether/whether


https://reviews.llvm.org/D26837