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

[Chris Hamilton via Phabricator via cfe-commits]

chrish_ericsson_atx marked 2 inline comments 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();
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86796/new/

https://reviews.llvm.org/D86796