AdamMagierFOSS added a comment.
Thank you for the feedback - I've added responses inline and I'll update the
change to reflect the feedback.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2247
// Otherwise, evaluate and record it.
- if (const Expr *size = vat->getSizeExpr()) {
+ if (const Expr *SizeExpr = vat->getSizeExpr()) {
// It's possible that we might have emitted this already,
----------------
rjmccall wrote:
> Please continue to use lowercase names for local variables for consistency
> with the surrounding code. The rename to `sizeExpr` is good; you can then
> rename `Size` to `size` or `sizeValue`.
Thank you for clarifying. I was unsure which style to stick to since I saw both
"new style" code and "old style" code within the same scope, so I went with the
"new style" to align with recent submissions to the clang project I looked at.
It's no problem at all for me to switch my changes to the "old style" if that's
preferred in this instance.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2259
// greater than zero.
- if (SanOpts.has(SanitizerKind::VLABound) &&
- size->getType()->isSignedIntegerType()) {
+ if (SanOpts.has(SanitizerKind::VLABound) && SEType->isIntegerType())
{
SanitizerScope SanScope(this);
----------------
rjmccall wrote:
> Sema requires the bound expression to have integral type, so you don't need
> to do this.
I suspected this would be unnecessary - thank you for confirming, I'll remove
this.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2275
// undefined behavior to have a negative bound.
- entry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
+ MapEntry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
}
----------------
rjmccall wrote:
> This would be a different bug, but should UBSan also be doing a bounds check
> if the type is larger than `size_t`?
Interesting point, I'd have to reread through the spec to give a
precise/definitive answer. To keep this review focused I'll table the
discussion for a separate forum.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116048/new/
https://reviews.llvm.org/D116048
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
