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