Skip to content
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

[clang][Sema] Warn on self move for inlined static cast #76646

Merged
merged 4 commits into from
Mar 6, 2024

Conversation

MaxEW707
Copy link
Contributor

There are code bases that inline std::move manually via static_cast.
Treat a static cast to an xvalue as an inlined std::move call and warn on a self move.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Dec 31, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Dec 31, 2023

@llvm/pr-subscribers-clang

Author: Max Winkler (MaxEW707)

Changes

There are code bases that inline std::move manually via static_cast.
Treat a static cast to an xvalue as an inlined std::move call and warn on a self move.


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

2 Files Affected:

  • (modified) clang/lib/Sema/SemaChecking.cpp (+14-5)
  • (modified) clang/test/SemaCXX/warn-self-move.cpp (+13)
diff --git a/clang/lib/Sema/SemaChecking.cpp b/clang/lib/Sema/SemaChecking.cpp
index 2a69325f029514..a21410434d8099 100644
--- a/clang/lib/Sema/SemaChecking.cpp
+++ b/clang/lib/Sema/SemaChecking.cpp
@@ -18843,17 +18843,26 @@ void Sema::DiagnoseSelfMove(const Expr *LHSExpr, const Expr *RHSExpr,
   LHSExpr = LHSExpr->IgnoreParenImpCasts();
   RHSExpr = RHSExpr->IgnoreParenImpCasts();
 
-  // Check for a call expression
+  // Check for a call expression or static_cast expression
   const CallExpr *CE = dyn_cast<CallExpr>(RHSExpr);
-  if (!CE || CE->getNumArgs() != 1)
+  const CXXStaticCastExpr *CXXSCE = dyn_cast<CXXStaticCastExpr>(RHSExpr);
+  if (!CE && !CXXSCE)
     return;
 
   // Check for a call to std::move
-  if (!CE->isCallToStdMove())
+  if (CE && (CE->getNumArgs() != 1 || !CE->isCallToStdMove()))
     return;
 
-  // Get argument from std::move
-  RHSExpr = CE->getArg(0);
+  // Check for a static_cast<T&&>(..) to an xvalue which we can treat as an
+  // inlined std::move
+  if (CXXSCE && !CXXSCE->isXValue())
+    return;
+
+  // Get argument from std::move or static_cast
+  if (CE)
+    RHSExpr = CE->getArg(0);
+  else
+    RHSExpr = CXXSCE->getSubExpr();
 
   const DeclRefExpr *LHSDeclRef = dyn_cast<DeclRefExpr>(LHSExpr);
   const DeclRefExpr *RHSDeclRef = dyn_cast<DeclRefExpr>(RHSExpr);
diff --git a/clang/test/SemaCXX/warn-self-move.cpp b/clang/test/SemaCXX/warn-self-move.cpp
index 0987e9b6bf6017..d0158626424142 100644
--- a/clang/test/SemaCXX/warn-self-move.cpp
+++ b/clang/test/SemaCXX/warn-self-move.cpp
@@ -16,6 +16,9 @@ void int_test() {
   x = std::move(x);  // expected-warning{{explicitly moving}}
   (x) = std::move(x);  // expected-warning{{explicitly moving}}
 
+  x = static_cast<int&&>(x);  // expected-warning{{explicitly moving}}
+  (x) = static_cast<int&&>(x);  // expected-warning{{explicitly moving}}
+
   using std::move;
   x = move(x); // expected-warning{{explicitly moving}} \
                    expected-warning {{unqualified call to 'std::move}}
@@ -26,6 +29,9 @@ void global_int_test() {
   global = std::move(global);  // expected-warning{{explicitly moving}}
   (global) = std::move(global);  // expected-warning{{explicitly moving}}
 
+  global = static_cast<int&&>(global);  // expected-warning{{explicitly moving}}
+  (global) = static_cast<int&&>(global);  // expected-warning{{explicitly moving}}
+
   using std::move;
   global = move(global); // expected-warning{{explicitly moving}} \
                              expected-warning {{unqualified call to 'std::move}}
@@ -35,11 +41,14 @@ class field_test {
   int x;
   field_test(field_test&& other) {
     x = std::move(x);  // expected-warning{{explicitly moving}}
+    x = static_cast<int&&>(x);  // expected-warning{{explicitly moving}}
     x = std::move(other.x);
     other.x = std::move(x);
     other.x = std::move(other.x);  // expected-warning{{explicitly moving}}
+    other.x = static_cast<int&&>(other.x);  // expected-warning{{explicitly moving}}
   }
   void withSuggest(int x) {
+    x = static_cast<int&&>(x); // expected-warning{{explicitly moving variable of type 'int' to itself; did you mean to move to member 'x'?}}
     x = std::move(x); // expected-warning{{explicitly moving variable of type 'int' to itself; did you mean to move to member 'x'?}}
   }
 };
@@ -50,11 +59,15 @@ struct C { C() {}; ~C() {} };
 void struct_test() {
   A a;
   a = std::move(a);  // expected-warning{{explicitly moving}}
+  a = static_cast<A&&>(a);  // expected-warning{{explicitly moving}}
 
   B b;
   b = std::move(b);  // expected-warning{{explicitly moving}}
+  b = static_cast<B&&>(b);  // expected-warning{{explicitly moving}}
   b.a = std::move(b.a);  // expected-warning{{explicitly moving}}
+  b.a = static_cast<A&&>(b.a);  // expected-warning{{explicitly moving}}
 
   C c;
   c = std::move(c);  // expected-warning{{explicitly moving}}
+  c = static_cast<C&&>(c);  // expected-warning{{explicitly moving}}
 }

@MaxEW707
Copy link
Contributor Author

Ping

Copy link
Collaborator

@shafik shafik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, any objections @tbaederr

clang/test/SemaCXX/warn-self-move.cpp Show resolved Hide resolved
Copy link
Contributor

@tbaederr tbaederr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better, thanks.

@MaxEW707
Copy link
Contributor Author

MaxEW707 commented Mar 6, 2024

Thanks all for the review :).

I will need one of you to commit on my behalf since I do not have write access.

@tbaederr tbaederr merged commit d9d9301 into llvm:main Mar 6, 2024
4 checks passed
@MaxEW707 MaxEW707 deleted the mew/static-cast-self-move-warning branch March 7, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants