ebevhan added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:13981
+ bool overflow;
+ llvm::APInt product(index);
+ product += 1;
----------------
What if index is wider than AddrBits, but the active bits are fewer? Then you
might miss out on triggering the overflow case in the multiplication.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:13992
+ MaxElems =
+ MaxElems.zext(std::max(AddrBits << 1, apElemBytes.getBitWidth()));
+ MaxElems += 1;
----------------
Should this not be AddrBits + 1 if you want to add 1 below?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:13993
+ MaxElems.zext(std::max(AddrBits << 1, apElemBytes.getBitWidth()));
+ MaxElems += 1;
+ if (MaxElems.getBitWidth() < apElemBytes.getBitWidth())
----------------
Though, why is the +1 here? Isn't this already the maximum number of elements?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:13994-13997
+ if (MaxElems.getBitWidth() < apElemBytes.getBitWidth())
+ MaxElems = MaxElems.zext(apElemBytes.getBitWidth());
+ else if (apElemBytes.getBitWidth() < MaxElems.getBitWidth())
+ apElemBytes = apElemBytes.zext(MaxElems.getBitWidth());
----------------
MaxElems should already be at a sufficient width here because of the earlier
max. You can probably just do apElemBytes =
apElemBytes.zextOrTrunc(MaxElems.getBitWidth())?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:13966
if (index.isUnsigned() || !index.isNegative()) {
- // It is possible that the type of the base expression after
- // IgnoreParenCasts is incomplete, even though the type of the base
- // expression before IgnoreParenCasts is complete (see PR39746 for an
- // example). In this case we have no information about whether the array
- // access exceeds the array bounds. However we can still diagnose an array
- // access which precedes the array bounds.
- if (BaseType->isIncompleteType())
- return;
+ if (isUnboundedArray) {
+ const auto &ASTC = getASTContext();
----------------
chrish_ericsson_atx wrote:
> ebevhan wrote:
> > It might simplify the patch to move this condition out of the tree and just
> > early return for the other case. That is:
> >
> > ```
> > if (isUnboundedArray) {
> > if (!(index.isUnsigned() || !index.isNegative()))
> > return;
> >
> > ...
> > return;
> > }
> >
> > if (index.isUnsigned() ...
> > ```
> There's a bit more code (starting at line 14094 in this patch set) that
> applies in all cases, so an early return here would prevent the "Array
> declared here" note from being generated.
Ah, the note.
I wonder if it wouldn't be cleaner (and avoid indenting the entire block) if
that was just duplicated.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86796/new/
https://reviews.llvm.org/D86796
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
