chrish_ericsson_atx marked an inline comment as done.
chrish_ericsson_atx added inline comments.
================
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:
> > 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.
> Hard to say which is cleaner -- it's a tradeoff. Nesting level would be
> shallower if that code was duplicated, but then again, the duplication
> increases the chance of an incomplete fix should an issue be discovered in
> that code. Overall code would be slightly longer, as well (adding about 16
> lines of code, but removing only 4). To me, the current strategy feels more
> surgical, but I'll change it if you feel more strongly about it than I do.
> Please re-ping if you do.
I reconsidered and made the changes you suggested. Thanks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D86796/new/
https://reviews.llvm.org/D86796
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
