[PATCH] D45774: [analyzer] cover more cases where a Loc can be bound to constants

[Artem Dergachev via Phabricator via cfe-commits]

NoQ added a comment.

Looks great, thanks!

> CXXDynamicCastExpr: I don't think evalCast would handle this correctly, does it?

Seems so. It is partially supported by `StoreManager::attemptDownCast()`. I also see relatively little urgency in handling it here because the whole point of `dynamic_cast` is to be unknown in compile-time; why would anybody need to dynamic_cast a compile-time null pointer?

Also for what-does-what sorts of answers you can consult the code in `ExprEngine::Visit*` which is essentially a huge switch by statement kinds.

> ObjCBridgedCastExpr: I don't know ObjectiveC

Same here, it's quite pointless to have a constant null pointer casted from one language to another.

> CastKinds for floating point: Floats cannot yet be handled by the analyzer, right?

Yep, that's right.

In https://reviews.llvm.org/D45774#1071666, @MTC wrote:

> Test files for `initialization` missing? : )


Yep, seems so. This is stuff like

  struct S {
    int x = 0;
  };

I'm not sure it even needs to be supported explicitly. If the field is static, it most likely acts like a simple global variable (i.e. a `VarDecl`, not a `FieldDecl`). If the field is non-static, we should pick-up the in-class initializer in the constructor. Also if it's non-static and const, it is unlikely to have a constant initializer, because why would anybody copy the same constant into every instance of the class.



================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1638
+      if (const Expr *Init = VD->getInit()) {
+        if (const InitListExpr *InitList = dyn_cast<InitListExpr>(Init)) {
+          // The array index has to be known.
----------------
Using `const auto *` is recommended with `dyn_cast` (also a plain `auto` with `getAs` and such), so that not to write down the type twice. We have a lot of old code around, but the new code should probably follow this recommendation.


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1642-1645
+            // Return unknown value if index is out of bounds.
+            if (i < 0 || i >= InitList->getNumInits()) {
+              return UnknownVal();
+            }
----------------
Hmm, we might as well want to return an `UndefinedVal()` when the index is out of bounds of the actual array. The size of the array can be retrieved from `VD`. Though if you decide to do that, please put it into a separate patch :)


================
Comment at: lib/StaticAnalyzer/Core/RegionStore.cpp:1723
+        if (const InitListExpr *InitList = dyn_cast<InitListExpr>(Init)) {
+          if (const Expr *FieldInit = InitList->getInit(FD->getFieldIndex())) {
+            if (Optional<SVal> V = svalBuilder.getConstantVal(FieldInit))
----------------
This method crashes when the index is out of range. Not sure - is it possible to skip a few fields in the list? Would the list's AST automatically contain any placeholders in this case?


Repository:
  rC Clang

https://reviews.llvm.org/D45774