aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM!
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:8841-8844
+def warn_array_index_exceeds_max_addressable_bounds : Warning<
+ "array index %0 refers past the last possible element for an array in %1-bit
"
+ "address space containing %2-bit (%3-byte) elements (max possible %4
element%s5)">,
+ InGroup<ArrayBounds>;
----------------
chrish_ericsson_atx wrote:
> aaron.ballman wrote:
> > I'd combine this with the above diagnostic given how similar they are:
> > ```
> > def warn_exceeds_max_addressable_bounds : Warning<
> > "%select{array index %1|the pointer incremented by %1}0 refers past the
> > last possible element for an array in %2-bit "
> > "address space containing %3-bit (%4-byte) elements (max possible %5
> > element%s6)">,
> > InGroup<ArrayBounds>;
> > ```
> I was attempting to follow the pattern set by the preceding four definitions,
> which keep pointer math warnings and array index warnings separated, but are
> otherwise nearly identical. If there's value in that pattern for those other
> warnings, I would assume the same value applies to keeping these separated.
> Please re-ping if you disagree, otherwise, I'll leave these as two separate
> warnings.
I don't think there's a whole lot of value, but I also don't insist on the
change.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:14063
+ if (isUnboundedArray) {
+ if (index.isUnsigned() || !index.isNegative()) {
+ const auto &ASTC = getASTContext();
----------------
chrish_ericsson_atx wrote:
> aaron.ballman wrote:
> > ebevhan wrote:
> > > This could be early return to avoid the indentation.
> > +1 to using early return here. I might even get rid of `isUnboundedArray`
> > and just use `!BaseType`
> @ebevhan's comment about early return was actually addressed in a previous
> diff... I should have marked it as such. My apologies.
>
> I'd prefer to keep `isUnboundedArray` (with the case corrected, of course),
> as it seems much clearer than `!BaseType` in terms of expressing what we are
> actually checking here, and why.
That's fine by me, and my brain scrambled a bit -- I thought there was an early
return possible for `!IsUnboundedArray` but I was looking at the wrong line.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:14100
+ // dependent CharUnits)
+ DiagRuntimeBehavior(BaseExpr->getBeginLoc(), BaseExpr,
+ PDiag(DiagID)
----------------
chrish_ericsson_atx wrote:
> aaron.ballman wrote:
> > It's not clear to me whether we need to pay the extra expense of doing
> > reachability checks for the diagnostic -- do you have evidence that there
> > would be a high false positive rate if we always emit the diagnostic?
> Sorry-- I'm not following you here, @aaron.ballman. What reachability check
> are you referring to? The diagnostic isn't conditioned on anything (at least
> not here, where your comment appears...), so I'm not sure what change you are
> suggesting that would "always emit the diagnostic"... Sorry to be slow on
> the uptake here... Could you clarify that for me?
`DiagRuntimeBehavior()` does a reachability check on the given expression and
will not emit the diagnostic if the code is not reachable. This check can be
somewhat expensive and so it's usually only used when we need to eliminate
false positives in common code patterns.
Instead of using `DiagRuntimeBehavior()`, you could use `Diag()` instead which
doesn't do the reachability check. However, looking further at the changes, I
think you're correct to use `DiagRuntimeBehavior()` -- we use it elsewhere in
this same function. Sorry for the noise!
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
