From d25e24a0eb909b7604572d28d15cbe648ecccd90 Mon Sep 17 00:00:00 2001 From: Carlos Galvez Date: Wed, 4 Sep 2024 11:49:16 +0200 Subject: [PATCH] =?UTF-8?q?[clang-tidy]=20Suggest=20using=20reinterpret=5F?= =?UTF-8?q?cast=20in=20bugprone-casting-thro=E2=80=A6=20(#106784)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …ugh-void reinterpret_cast is the equivalent construct, and more clearly expresses intent. Co-authored-by: Carlos Gálvez --- .../bugprone/CastingThroughVoidCheck.cpp | 4 +- clang-tools-extra/docs/ReleaseNotes.rst | 4 ++ .../checks/bugprone/casting-through-void.rst | 21 +++++++++- .../bugprone/casting-through-void.cpp | 40 +++++++++---------- 4 files changed, 46 insertions(+), 23 deletions(-) diff --git a/clang-tools-extra/clang-tidy/bugprone/CastingThroughVoidCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/CastingThroughVoidCheck.cpp index 9e714b4be4dfea..f0a9ace2297406 100644 --- a/clang-tools-extra/clang-tidy/bugprone/CastingThroughVoidCheck.cpp +++ b/clang-tools-extra/clang-tidy/bugprone/CastingThroughVoidCheck.cpp @@ -38,7 +38,9 @@ void CastingThroughVoidCheck::check(const MatchFinder::MatchResult &Result) { const auto ST = *Result.Nodes.getNodeAs("source_type"); const auto VT = *Result.Nodes.getNodeAs("void_type"); const auto *CE = Result.Nodes.getNodeAs("cast"); - diag(CE->getExprLoc(), "do not cast %0 to %1 through %2") << ST << TT << VT; + diag(CE->getExprLoc(), + "do not cast %0 to %1 through %2; use reinterpret_cast instead") + << ST << TT << VT; } } // namespace clang::tidy::bugprone diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index b001a6ad446695..6999c1ef2ea4b0 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -104,6 +104,10 @@ New check aliases Changes in existing checks ^^^^^^^^^^^^^^^^^^^^^^^^^^ +- Improved :doc:`bugprone-casting-through-void + ` check to suggest replacing + the offending code with ``reinterpret_cast``, to more clearly express intent. + - Improved :doc:`modernize-use-std-format ` check to support replacing member function calls too. diff --git a/clang-tools-extra/docs/clang-tidy/checks/bugprone/casting-through-void.rst b/clang-tools-extra/docs/clang-tidy/checks/bugprone/casting-through-void.rst index a9ab478b9a82e1..d9f94b6a3f20b9 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/bugprone/casting-through-void.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/bugprone/casting-through-void.rst @@ -3,7 +3,9 @@ bugprone-casting-through-void ============================= -Detects unsafe or redundant two-step casting operations involving ``void*``. +Detects unsafe or redundant two-step casting operations involving ``void*``, +which is equivalent to ``reinterpret_cast`` as per the +`C++ Standard `_. Two-step type conversions via ``void*`` are discouraged for several reasons. @@ -16,7 +18,17 @@ Two-step type conversions via ``void*`` are discouraged for several reasons. In summary, avoiding two-step type conversions through ``void*`` ensures clearer code, maintains essential compiler warnings, and prevents ambiguity and potential runtime -errors, particularly in complex inheritance scenarios. +errors, particularly in complex inheritance scenarios. If such a cast is wanted, +it shall be done via ``reinterpret_cast``, to express the intent more clearly. + +Note: it is expected that, after applying the suggested fix and using +``reinterpret_cast``, the check :doc:`cppcoreguidelines-pro-type-reinterpret-cast +<../cppcoreguidelines/pro-type-reinterpret-cast>` will emit a warning. +This is intentional: ``reinterpret_cast`` is a dangerous operation that can +easily break the strict aliasing rules when dereferencing the casted pointer, +invoking Undefined Behavior. The warning is there to prompt users to carefuly +analyze whether the usage of ``reinterpret_cast`` is safe, in which case the +warning may be suppressed. Examples: @@ -29,3 +41,8 @@ Examples: reinterpret_cast(reinterpret_cast(ptr)); // WRONG (IntegerPointer)(void *)ptr; // WRONG IntegerPointer(static_cast(ptr)); // WRONG + + reinterpret_cast(ptr); // OK, clearly expresses intent. + // NOTE: dereferencing this pointer violates + // the strict aliasing rules, invoking + // Undefined Behavior. diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/casting-through-void.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/casting-through-void.cpp index a784e498858738..68172212904f8a 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/casting-through-void.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/casting-through-void.cpp @@ -10,42 +10,42 @@ const double cd = 100; void normal_test() { static_cast(static_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'V' (aka 'void *') [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'V' (aka 'void *'); use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&i)); - // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'int *' to 'int *' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'int *' to 'int *' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&i)); } void const_pointer_test() { static_cast(static_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'double *' to 'int *const' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'double *' to 'int *const' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'double *' to 'int *const' through 'V' (aka 'void *') [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'double *' to 'int *const' through 'V' (aka 'void *'); use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&i)); - // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'int *' to 'int *const' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'int *' to 'int *const' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&i)); } void const_test() { static_cast(static_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'double *' to 'const int *' through 'const void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'double *' to 'const int *' through 'const void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'double *' to 'const int *' through 'const V' (aka 'void *const') [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'double *' to 'const int *' through 'const V' (aka 'void *const'); use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&i)); - // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'int *' to 'const int *' through 'const void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'int *' to 'const int *' through 'const void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&i)); static_cast(static_cast(&cd)); - // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'const double *' to 'const int *' through 'const void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'const double *' to 'const int *' through 'const void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&cd)); - // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'const double *' to 'const int *' through 'const CV' (aka 'const void *const') [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'const double *' to 'const int *' through 'const CV' (aka 'const void *const'); use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&ci)); - // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'const int *' to 'const int *' through 'const void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: do not cast 'const int *' to 'const int *' through 'const void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast(static_cast(&ci)); } @@ -53,11 +53,11 @@ void const_test() { void reinterpret_cast_test() { static_cast(reinterpret_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] reinterpret_cast(static_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'double *' to 'int *' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'double *' to 'int *' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] reinterpret_cast(reinterpret_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'double *' to 'int *' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: do not cast 'double *' to 'int *' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast(reinterpret_cast(&i)); reinterpret_cast(reinterpret_cast(&i)); @@ -66,11 +66,11 @@ void reinterpret_cast_test() { void c_style_cast_test() { static_cast((void *)&d); - // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] (int *)(void *)&d; - // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: do not cast 'double *' to 'int *' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: do not cast 'double *' to 'int *' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast((void *)&d); - // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: do not cast 'double *' to 'int *' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] static_cast((void *)&i); } @@ -82,12 +82,12 @@ using I = int *; void cxx_functional_cast() { A(static_cast(&d)); I(static_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not cast 'double *' to 'I' (aka 'int *') through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: do not cast 'double *' to 'I' (aka 'int *') through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] } void bit_cast() { __builtin_bit_cast(int *, static_cast(&d)); - // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: do not cast 'double *' to 'int *' through 'void *' [bugprone-casting-through-void] + // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: do not cast 'double *' to 'int *' through 'void *'; use reinterpret_cast instead [bugprone-casting-through-void] } namespace PR87069 {