[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 =
----------------
ziqingluo-90 wrote:
> jkorous wrote:
> > 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);
> > }
> > ```
> I think we are fine.  According to the doc of `FunctionDecl`:
> ```
> /// Represents a function declaration or definition.
> ///
> /// Since a given function can be declared several times in a program,
> /// there may be several FunctionDecls that correspond to that
> /// function. Only one of those FunctionDecls will be found when
> /// traversing the list of declarations in the context of the
> /// FunctionDecl (e.g., the translation unit); this FunctionDecl
> /// contains all of the information known about the function. Other,
> /// previous declarations of the function are available via the
> /// getPreviousDecl() chain.
> ```
I see! Sound like we should be fine indeed and the test seems to confirm.
Thank you!


================
Comment at: clang/lib/Analysis/UnsafeBufferUsage.cpp:508
+                      hasUnaryOperand(arraySubscriptExpr(
+                          hasBase(ignoringParenImpCasts(declRefExpr())))))
+            .bind(UPCAddressofArraySubscriptTag)))));
----------------
ziqingluo-90 wrote:
> 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]`.
Oh, my bad! I assumed (AKA didn't check) that we're just replacing the parts of the code around the DRE and index.
You're right. Please ignore me :)


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

https://reviews.llvm.org/D143128