[PATCH] D104285: [analyzer][AST] Retrieve value by direct index from list initialization of constant array declaration.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/include/clang/AST/Expr.h:4959
 
+  /// Return an value-expression under the given index.
+  ///
----------------



================
Comment at: clang/include/clang/AST/Expr.h:4970
+  /// - `this` if there's no expression for the valid index;
+  /// - `nullptr` for invalid index (`i < 0` or `i >= array_size`)
+  ///   or if it is not a list for constant array type.
----------------
`i` cannot be `< 0` because the index here is unsigned anyway.


================
Comment at: clang/include/clang/AST/Expr.h:4973-4975
+  /// This version adapted to treat unsigned integer to distinguish between
+  /// -1 and ULONG_LONG_MAX.
+  const Expr *getExprForConstArrayByRawIndex(int64_t Idx) const;
----------------
I don't think this overload adds enough value -- the indexes are naturally unsigned, and the caller should validate the behavior if the source expression is signed.


================
Comment at: clang/lib/AST/Expr.cpp:2354-2358
+  if (!isa<ConstantArrayType>(T))
+    return nullptr;
+
+  SmallVector<uint64_t, 2> Extents =
+      cast<ConstantArrayType>(T)->getAllExtents();
----------------
Hmm, generally speaking, you should not cast an arbitrary type to an array type because that won't do the correct thing for qualifiers. Instead, you'd usually use `ASTContext::getAsConstantArrayType()` to get the correct type. However, because you're just getting the array extent, I don't believe that can be impacted. However, `isa` followed by `cast` is a code smell, and that should at least be using a `dyn_cast`.

@rsmith, do you have thoughts on this?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104285/new/

https://reviews.llvm.org/D104285