[PATCH] D77836: [Attribute] Fix noderef attribute false-negatives

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/SemaCast.cpp:734
 void CastOperation::CheckDynamicCast() {
+  CheckNoDerefRAII noderef_check(*this);
+
----------------
Please use UpperCamelCase for local variables.


================
Comment at: clang/lib/Sema/SemaCast.cpp:2557-2558
           return;
+        CheckNoDeref(Self, SrcExpr.get()->getType(), ResultType,
+                     OpRange.getBegin());
       }
----------------
Should we be checking this for a C-style cast?  You previously said  you were going to turn off the checks for C-style casts. Did you change your mind on that?


================
Comment at: clang/lib/Sema/SemaCast.cpp:2982
 
+  CheckNoDeref(*this, Op.SrcExpr.get()->getType(), Op.ResultType, LPLoc);
+
----------------
As above, should we be checking this for a C-style cast? (It also looks like we're doing this check twice in C++.)


================
Comment at: clang/lib/Sema/SemaInit.cpp:8182-8184
+            // Do not check static casts here because they are checked earlier
+            // in Sema::ActOnCXXNamedCast()
+            if (!Kind.isStaticCast()) {
----------------
leonardchan wrote:
> leonardchan wrote:
> > rsmith wrote:
> > > Are there any `static_cast` cases that don't get here? I'm also a little surprised that `static_cast` would get here but the `static_cast` interpretation of a C-style or functional cast would not.
> > I'll double check on this. The reason I added this here was because the cast in `static_cast_from_same_ptr_type()` triggered 2 of the `warn_noderef_to_dereferenceable_pointer` warnings and one of them was triggered here.
> Yeah it seems the `int *a = static_cast<int *>(x);` case in `cast_from_void_ptr()` is an example of a static_cast that doesn't go through here.
I see, this is only reached for the part of `static_cast` that considers implicit conversion sequences. OK.

I expect there's similar overlap for all the other kinds of cast too. Should we be suppressing this check for *all* casts, given that we perform the check in `SemaCast` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77836