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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
