noderef was failing to trigger warnings in some cases related to c++ style casting. This patch addresses them.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Sema/SemaCast.cpp | ||
---|---|---|
275 | 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. | |
clang/lib/Sema/SemaInit.cpp | ||
8182–8189 | 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. |
clang/test/Frontend/noderef.c | ||
---|---|---|
211 ↗ | (On Diff #256428) | The name of the function is a bit odd given that there's an explicit cast. |
clang/lib/Sema/SemaCast.cpp | ||
---|---|---|
275 | 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. | |
clang/lib/Sema/SemaInit.cpp | ||
8182–8189 | 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. | |
clang/test/Frontend/noderef.c | ||
211 ↗ | (On Diff #256428) | Woops. Will update the name. |
Updated to address comments and added an RAII class for easier checking before exiting functions.
clang/lib/Sema/SemaInit.cpp | ||
---|---|---|
8182–8189 | 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. |
clang/lib/Sema/SemaCast.cpp | ||
---|---|---|
734 | Please use UpperCamelCase for local variables. | |
2557–2558 | 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? | |
2980 | As above, should we be checking this for a C-style cast? (It also looks like we're doing this check twice in C++.) | |
clang/lib/Sema/SemaInit.cpp | ||
8182–8189 | 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? |
clang/lib/Sema/SemaCast.cpp | ||
---|---|---|
2557–2558 | No, you're right. I accidentally left this in. | |
2980 | Also accidentally left this in. | |
clang/lib/Sema/SemaInit.cpp | ||
8182–8189 | Hmm. I think for this particular location, static_cast contexts are all we need to suppress it for to prevent duplicate warnings from showing. We don't need to suppress for these since the attribute is allowed to pass through these: IC_CStyleCast IC_FunctionalCast We perform the check for implicit casts here since (AFAICT) only explicit casts (C-style, functional, and C++ style) seem to be covered in SemaCast: IC_Implicit This would cover cases like int *ptr = noderef_ptr;. I'm not sure about these ones, but none of the cases I have seem to fail if I suppress only them: IC_Normal IC_ExplicitConvs It could be though that there's a particular case I'm not covering in my tests. |
Lemme know if there's any more feedback. Will aim for committing this sometime at the end of the day.
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.