[PATCH] D104285: [analyzer][AST] Retrieve value by direct index from list initialization of constant array declaration.
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 5 06:13:13 PDT 2021
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
More information about the cfe-commits
mailing list