chrish_ericsson_atx added a comment.
Thank you for the comments, @aaron.ballman . I'll update with the changes you
requested shortly. I did have some requests for clarification of you, though.
Thanks!
================
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>;
----------------
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.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:14063
+ if (isUnboundedArray) {
+ if (index.isUnsigned() || !index.isNegative()) {
+ const auto &ASTC = getASTContext();
----------------
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.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:14100
+ // dependent CharUnits)
+ DiagRuntimeBehavior(BaseExpr->getBeginLoc(), BaseExpr,
+ PDiag(DiagID)
----------------
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?
================
Comment at: clang/lib/Sema/SemaChecking.cpp:14111
+ // Try harder to find a NamedDecl to point at in the note.
+ while (const ArraySubscriptExpr *ASE =
+ dyn_cast<ArraySubscriptExpr>(BaseExpr))
----------------
aaron.ballman wrote:
> You can use `const auto *` here since the type is spelled out in the
> initialization. Same below.
Fair point -- this was just cut-and-paste from the code I had to duplicate in
order to address the early return comment from @ebevhan. :) I'll change it in
both places.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:14121
+ if (ND)
+ DiagRuntimeBehavior(ND->getBeginLoc(), BaseExpr,
+ PDiag(diag::note_array_declared_here) << ND);
----------------
aaron.ballman wrote:
> Similar comment here about reachability.
Similar request for clarification. ;)
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
