[PATCH] D86796: [Sema] Address-space sensitive index check for unbounded arrays

[Chris Hamilton via Phabricator via cfe-commits]

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