Skip to content

[clang-tidy] Suggest using reinterpret_cast in bugprone-casting-thro… #106784

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged

Conversation

carlosgalvezp
Copy link
Contributor

…ugh-void

reinterpret_cast is the equivalent construct, and more clearly expresses intent.

@llvmbot
Copy link
Member

llvmbot commented Aug 30, 2024

@llvm/pr-subscribers-clang-tidy

@llvm/pr-subscribers-clang-tools-extra

Author: Carlos Galvez (carlosgalvezp)

Changes

…ugh-void

reinterpret_cast is the equivalent construct, and more clearly expresses intent.


Full diff: https://github.com/llvm/llvm-project/pull/106784.diff

4 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/bugprone/CastingThroughVoidCheck.cpp (+3-1)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
  • (modified) clang-tools-extra/docs/clang-tidy/checks/bugprone/casting-through-void.rst (+18-2)
  • (modified) clang-tools-extra/test/clang-tidy/checkers/bugprone/casting-through-void.cpp (+20-20)
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<QualType>("source_type");
   const auto VT = *Result.Nodes.getNodeAs<QualType>("void_type");
   const auto *CE = Result.Nodes.getNodeAs<ExplicitCastExpr>("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
+  <clang-tidy/checks/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
   <clang-tidy/checks/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..05125ae7bdc8ee 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 <https://eel.is/c++draft/expr.reinterpret.cast#7>`_.
 
 Two-step type conversions via ``void*`` are discouraged for several reasons.
 
@@ -16,7 +18,16 @@ 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 ``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 +40,8 @@ Examples:
    reinterpret_cast<IntegerPointer>(reinterpret_cast<void *>(ptr)); // WRONG
    (IntegerPointer)(void *)ptr; // WRONG
    IntegerPointer(static_cast<void *>(ptr)); // WRONG
+
+   reinterpret_cast<IntegerPointer>(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<int *>(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<int *>(static_cast<V>(&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<int *>(static_cast<void *>(&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<void *>(static_cast<void *>(&i));
 }
 
 void const_pointer_test() {
   static_cast<int *const>(static_cast<void *>(&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<int *const>(static_cast<V>(&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<int *const>(static_cast<void *>(&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<void *const>(static_cast<void *>(&i));
 }
 
 void const_test() {
   static_cast<const int *>(static_cast<const void *>(&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<const int *>(static_cast<const V>(&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<const int *>(static_cast<const void *>(&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<const void *>(static_cast<const void *>(&i));
 
   static_cast<const int *>(static_cast<const void *>(&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<const int *>(static_cast<const CV>(&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<const int *>(static_cast<const void *>(&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<const void *>(static_cast<const void *>(&ci));
 }
@@ -53,11 +53,11 @@ void const_test() {
 
 void reinterpret_cast_test() {
   static_cast<int *>(reinterpret_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]
   reinterpret_cast<int *>(static_cast<void *>(&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<int *>(reinterpret_cast<void *>(&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<void *>(reinterpret_cast<void *>(&i));
   reinterpret_cast<void *>(reinterpret_cast<void *>(&i));
@@ -66,11 +66,11 @@ void reinterpret_cast_test() {
 
 void c_style_cast_test() {
   static_cast<int *>((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<int *>((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 *>((void *)&i);
 }
@@ -82,12 +82,12 @@ using I = int *;
 void cxx_functional_cast() {
   A(static_cast<void*>(&d));
   I(static_cast<void*>(&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<void *>(&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 {

…ugh-void

reinterpret_cast is the equivalent construct, and more clearly
expresses intent.
@carlosgalvezp carlosgalvezp force-pushed the casting_void_reinterpret_cast branch from d3f0a65 to d88a216 Compare September 3, 2024 09:05
@carlosgalvezp carlosgalvezp merged commit d25e24a into llvm:main Sep 4, 2024
9 checks passed
@carlosgalvezp carlosgalvezp deleted the casting_void_reinterpret_cast branch September 4, 2024 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants