[clang] [Sema] Simplify Expr::isUnusedResultAWarning for CXXConstructExpr

[halbi2]

…Expr

Two tests have new warnings because warn_unused_result is now respected for constructor temporaries. These tests were newly added in #112521 last year. This is good because the new behavior is better than the old.

@Sirraide and @Mick235711 what do you think about it?

[llvmbot]

@llvm/pr-subscribers-clang

Author: None (halbi2)

Changes

…Expr

Two tests have new warnings because warn_unused_result is now respected for constructor temporaries. These tests were newly added in #112521 last year. This is good because the new behavior is better than the old.

@Sirraide and @Mick235711 what do you think about it?

---

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

3 Files Affected:

• (modified) clang/lib/AST/Expr.cpp (+10-23)
• (modified) clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp (+1-1)
• (modified) clang/test/SemaCXX/warn-unused-result.cpp (+2-2)

diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 7cac655ef151c..e3a746069cd3f 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -2805,32 +2805,19 @@ bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
 
   case CXXTemporaryObjectExprClass:
   case CXXConstructExprClass: {
-    if (const CXXRecordDecl *Type = getType()->getAsCXXRecordDecl()) {
-      const auto *WarnURAttr = Type->getAttr<WarnUnusedResultAttr>();
-      if (Type->hasAttr<WarnUnusedAttr>() ||
-          (WarnURAttr && WarnURAttr->IsCXX11NoDiscard())) {
-        WarnE = this;
-        Loc = getBeginLoc();
-        R1 = getSourceRange();
-        return true;
-      }
-    }
-
     const auto *CE = cast<CXXConstructExpr>(this);
-    if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
-      const auto *WarnURAttr = Ctor->getAttr<WarnUnusedResultAttr>();
-      if (WarnURAttr && WarnURAttr->IsCXX11NoDiscard()) {
-        WarnE = this;
-        Loc = getBeginLoc();
-        R1 = getSourceRange();
+    const CXXRecordDecl *Type = getType()->getAsCXXRecordDecl();
 
-        if (unsigned NumArgs = CE->getNumArgs())
-          R2 = SourceRange(CE->getArg(0)->getBeginLoc(),
-                           CE->getArg(NumArgs - 1)->getEndLoc());
-        return true;
-      }
-    }
+    if ((Type && Type->hasAttr<WarnUnusedAttr>()) || CE->hasUnusedResultAttr(Ctx)) {
+      WarnE = this;
+      Loc = getBeginLoc();
+      R1 = getSourceRange();
 
+      if (unsigned NumArgs = CE->getNumArgs())
+        R2 = SourceRange(CE->getArg(0)->getBeginLoc(),
+                         CE->getArg(NumArgs - 1)->getEndLoc());
+      return true;
+    }
     return false;
   }
 
diff --git a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
index 0012ab976baa5..7f933a4dcc6b2 100644
--- a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp
@@ -115,7 +115,7 @@ void usage() {
   S();    // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
   S('A'); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
   S(1);
-  S(2.2);
+  S(2.2); // expected-warning {{ignoring temporary created by a constructor declared with 'gnu::warn_unused_result' attribute}}
   Y(); // expected-warning {{ignoring temporary of type 'Y' declared with 'nodiscard' attribute: Don't throw me away either!}}
   S s;
   ConvertTo{}; // expected-warning {{ignoring return value of type 'ConvertTo' declared with 'nodiscard' attribute: Don't throw me away!}}
diff --git a/clang/test/SemaCXX/warn-unused-result.cpp b/clang/test/SemaCXX/warn-unused-result.cpp
index 447654eccd563..1f7913f1aa994 100644
--- a/clang/test/SemaCXX/warn-unused-result.cpp
+++ b/clang/test/SemaCXX/warn-unused-result.cpp
@@ -309,7 +309,7 @@ void use() {
 
   S<double>(2);     // no warning
   S<int>(2);        // expected-warning {{ignoring temporary of type 'S<int>' declared with 'nodiscard'}}
-  S<const char>(2); // no warning (warn_unused_result does not diagnose constructor temporaries)
+  S<const char>(2); // expected-warning {{ignoring temporary of type 'S<const char>' declared with 'clang::warn_unused_result' attribute}}
 
   // function should take precedence over type
   obtain2(1.0);             // expected-warning {{ignoring return value of function declared with 'nodiscard'}}
@@ -336,7 +336,7 @@ struct [[nodiscard]] G {
 void use2() {
   H{2};       // no warning
   H(2.0);     // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard'}}
-  H("Hello"); // no warning (warn_unused_result does not diagnose constructor temporaries)
+  H("Hello"); // expected-warning {{ignoring temporary created by a constructor declared with 'warn_unused_result' attribute}}
 
   // no warning for explicit cast to void
   (void)H(2);

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[Sirraide]

I believe we might not be warning on warn_unused_result on purpose for GCC compatibility; I’m not sure if we want to change that.

[erichkeane]

I think the new warning behavior here makes sense. We rarely/never suppress warnings that make sense for compat reasons.

This also provides a way of dealing with things like std::lock_guard typos.

[halbi2]

@erichkeane will you land this on my behalf?

[erichkeane]

@erichkeane will you land this on my behalf?

Sure, done.