[PATCH] D143128: [-Wunsafe-buffer-usage][WIP] Fix-Its transforming `&DRE[any]` to `DRE.data() + any`

Mon Feb 6 15:05:55 PST 2023

ziqingluo-90 added inline comments.

================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:508
+                      hasUnaryOperand(arraySubscriptExpr(
+                          hasBase(ignoringParenImpCasts(declRefExpr())))))
+            .bind(UPCAddressofArraySubscriptTag)))));
----------------
jkorous wrote:
> I am wondering what will happen in the weird corner-case of `&5[ptr]` - I feel the Fix-It we produce would be incorrect.
> 
> Here's a suggestion - we could use `hasLHS` instead of `hasBase` here and add a FIXME that when we find the time we should also properly support the corner-case. That would be a pretty low-priority though - we definitely have more important patterns to support first.
> 
> WDYT?
> 
I'm not sure if I understand your concern.  For `&5[ptr]`, we will generate a fix-it `ptr.data() + 5` in cases `ptr` is assigned a `span` strategy.   It is same as the case of `&ptr[5]`.

================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:810
+      case Strategy::Kind::Span:
+        return fixUPCAddressofArraySubscriptWithSpan(Node);
+      case Strategy::Kind::Wontfix:
----------------
jkorous wrote:
> Since we use `std::nullopt` in `getFixits` to signal errors - we should either use the same strategy in `fixUPCAddressofArraySubscriptWithSpan` or translate the empty return value from it to `nullopt` here.
> (FWIWI I am leaning towards the former.)
> Forwarding the empty Fix-It would be incorrect.
> 
> 
Oh, that's a bug I made!  Thank you for finding it for me.

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

https://reviews.llvm.org/D143128