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

[Jan Korous via Phabricator via cfe-commits]

jkorous added inline comments.


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:162
+                   InnerMatcher)),
+               unless(callee(functionDecl(hasAttr(attr::UnsafeBufferUsage)))));
+  auto CastOperandMatcher =
----------------
I am just wondering how does the callee matcher work in situation with multiple re-declarations 🤔 

Something like this:
```
void foo(int* ptr);
[[clang::unsafe_buffer_usage]] void foo(int* ptr);
void foo(int* ptr);

void bar(int* ptr) {
  foo(ptr);
}
```


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:508
+                      hasUnaryOperand(arraySubscriptExpr(
+                          hasBase(ignoringParenImpCasts(declRefExpr())))))
+            .bind(UPCAddressofArraySubscriptTag)))));
----------------
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?



================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:810
+      case Strategy::Kind::Span:
+        return fixUPCAddressofArraySubscriptWithSpan(Node);
+      case Strategy::Kind::Wontfix:
----------------
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.




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

https://reviews.llvm.org/D143128