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

[Denys Petrov via Phabricator via cfe-commits]

ASDenysPetrov added a comment.

@aaron.ballman Thanks for the review and comments. I'll update it ASAP.



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


================
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.
----------------
aaron.ballman wrote:
> `i` cannot be `< 0` because the index here is unsigned anyway.
Aha, I see.


================
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;
----------------
aaron.ballman wrote:
> 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.
Sounds reasonable.


================
Comment at: clang/lib/AST/Expr.cpp:2354-2358
+  if (!isa<ConstantArrayType>(T))
+    return nullptr;
+
+  SmallVector<uint64_t, 2> Extents =
+      cast<ConstantArrayType>(T)->getAllExtents();
----------------
aaron.ballman wrote:
> 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?
I'll rewrite this part.


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

https://reviews.llvm.org/D104285