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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan marked 3 inline comments as done.
leonardchan added inline comments.


================
Comment at: clang/lib/Sema/SemaCast.cpp:264
 
+  CheckNoderef(*this, E->getType(), TInfo->getType(), OpLoc);
+
----------------
rsmith wrote:
> Warning on this seems reasonable for `static_cast` and `dynamic_cast`, but should `reinterpret_cast` (or the `reinterpret_cast` interpretation of a C-style cast) warn? Presumably we want to leave open some type system holes for the case where the programmer really does know what they're doing.
My initial idea for this was that users could explicitly disable the warning with `-Wno-noderef` or pragmas for line-level granularity, but I can see how this could be less convenient/not consistent with other casts. Will update to allow C-style + reinterpret_casts.


================
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()) {
----------------
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.


================
Comment at: clang/test/Frontend/noderef.c:211
+
+int *implicit_cast(NODEREF int *x) {
+  return (int *)x; // expected-warning{{casting to dereferenceable pointer removes 'noderef' attribute}}
----------------
aaron.ballman wrote:
> The name of the function is a bit odd given that there's an explicit cast.
Woops. Will update the name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77836