[clang] Improve diagnostic on [[nodiscard]] attribute #112521
Conversation
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Yihe Li (Mick235711) ChangesA follow-up and alternative proposal to #112289. In this PR, no new notes are added to avoid noise. Instead, originally, every struct S {};
struct [[nodiscard]] S2 {};
[[nodiscard]] S get1();
S2 get2();
int main() {
get1();
get2();
}The generated warning currently is the same for two calls in <source>:6:3: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
6 | get1();
| ^~~~
<source>:7:3: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
7 | get2();
| ^~~~
2 warnings generated.As suggested by #112289 (comment), this PR thus makes a distinction here by making test-3.cpp:6:3: warning: ignoring return value of function declared with 'nodiscard' attribute [-Werror,-Wunused-result]
6 | get1();
| ^~~~
test-3.cpp:7:3: warning: ignoring return value of type 'S2' declared with 'nodiscard' attribute [-Werror,-Wunused-value]
7 | get2();
| ^~~~
2 warnings generated.In addition to better clarity, this also helps identify which specialization is marked as For constructor calls, a different warning message is also added. Currently, for warning: ignoring temporary created by a constructor declared with 'nodiscard' attributeAfter this PR, if warning: ignoring temporary of type 'S' declared with 'nodiscard' attribute("created by a constructor" is removed since "declared with 'nodiscard'" applies to the type, not the constructor) One thing to note here is that for the following scenario: struct [[nodiscard]] S {
[[nodiscard]] S(int) {}
};
void use() { S(2); }The warning message is generated in the format for constructor placement ("ignoring temporary created by a constructor"). Similarly, for Compared to the original PR, Some new test cases are added at the end of Patch is 22.82 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112521.diff 8 Files Affected:
diff --git a/clang/include/clang/AST/Expr.h b/clang/include/clang/AST/Expr.h
index cbe62411d11bff..8f5679767529fd 100644
--- a/clang/include/clang/AST/Expr.h
+++ b/clang/include/clang/AST/Expr.h
@@ -3182,11 +3182,12 @@ class CallExpr : public Expr {
/// Returns the WarnUnusedResultAttr that is either declared on the called
/// function, or its return type declaration.
- const Attr *getUnusedResultAttr(const ASTContext &Ctx) const;
+ std::pair<const NamedDecl *, const Attr *>
+ getUnusedResultAttr(const ASTContext &Ctx) const;
/// Returns true if this call expression should warn on unused results.
bool hasUnusedResultAttr(const ASTContext &Ctx) const {
- return getUnusedResultAttr(Ctx) != nullptr;
+ return getUnusedResultAttr(Ctx).second != nullptr;
}
SourceLocation getRParenLoc() const { return RParenLoc; }
diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td
index c709795e7b21d8..f683bfe1df8dde 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9265,6 +9265,12 @@ def warn_unused_container_subscript_expr : Warning<
def warn_unused_call : Warning<
"ignoring return value of function declared with %0 attribute">,
InGroup<UnusedValue>;
+def warn_unused_return_type : Warning<
+ "ignoring %select{return value|temporary}0 of type %2 declared with %1 attribute">,
+ InGroup<UnusedValue>;
+def warn_unused_return_type_msg : Warning<
+ "ignoring %select{return value|temporary}0 of type %2 declared with %1 attribute: %3">,
+ InGroup<UnusedValue>;
def warn_unused_constructor : Warning<
"ignoring temporary created by a constructor declared with %0 attribute">,
InGroup<UnusedValue>;
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 9ecbf121e3fc0d..bbb05ec48523f5 100644
--- a/clang/lib/AST/Expr.cpp
+++ b/clang/lib/AST/Expr.cpp
@@ -1616,22 +1616,24 @@ QualType CallExpr::getCallReturnType(const ASTContext &Ctx) const {
return FnType->getReturnType();
}
-const Attr *CallExpr::getUnusedResultAttr(const ASTContext &Ctx) const {
+std::pair<const NamedDecl *, const Attr *>
+CallExpr::getUnusedResultAttr(const ASTContext &Ctx) const {
+ // If the callee is marked nodiscard, return that attribute
+ const Decl *D = getCalleeDecl();
+ if (const auto *A = D->getAttr<WarnUnusedResultAttr>())
+ return {nullptr, A};
+
// If the return type is a struct, union, or enum that is marked nodiscard,
// then return the return type attribute.
if (const TagDecl *TD = getCallReturnType(Ctx)->getAsTagDecl())
if (const auto *A = TD->getAttr<WarnUnusedResultAttr>())
- return A;
+ return {TD, A};
for (const auto *TD = getCallReturnType(Ctx)->getAs<TypedefType>(); TD;
TD = TD->desugar()->getAs<TypedefType>())
if (const auto *A = TD->getDecl()->getAttr<WarnUnusedResultAttr>())
- return A;
-
- // Otherwise, see if the callee is marked nodiscard and return that attribute
- // instead.
- const Decl *D = getCalleeDecl();
- return D ? D->getAttr<WarnUnusedResultAttr>() : nullptr;
+ return {TD->getDecl(), A};
+ return {nullptr, nullptr};
}
SourceLocation CallExpr::getBeginLoc() const {
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 9e235a46707cd4..4324a941706f02 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -204,22 +204,28 @@ static bool DiagnoseUnusedComparison(Sema &S, const Expr *E) {
return true;
}
-static bool DiagnoseNoDiscard(Sema &S, const WarnUnusedResultAttr *A,
- SourceLocation Loc, SourceRange R1,
- SourceRange R2, bool IsCtor) {
+static bool DiagnoseNoDiscard(Sema &S, const NamedDecl *OffendingDecl,
+ const WarnUnusedResultAttr *A, SourceLocation Loc,
+ SourceRange R1, SourceRange R2, bool IsCtor) {
if (!A)
return false;
StringRef Msg = A->getMessage();
if (Msg.empty()) {
+ if (OffendingDecl)
+ return S.Diag(Loc, diag::warn_unused_return_type)
+ << IsCtor << A << OffendingDecl << R1 << R2;
if (IsCtor)
return S.Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
return S.Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
}
+ if (OffendingDecl)
+ return S.Diag(Loc, diag::warn_unused_return_type_msg)
+ << IsCtor << A << OffendingDecl << Msg << R1 << R2;
if (IsCtor)
- return S.Diag(Loc, diag::warn_unused_constructor_msg) << A << Msg << R1
- << R2;
+ return S.Diag(Loc, diag::warn_unused_constructor_msg)
+ << A << Msg << R1 << R2;
return S.Diag(Loc, diag::warn_unused_result_msg) << A << Msg << R1 << R2;
}
@@ -290,9 +296,10 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S, unsigned DiagID) {
if (E->getType()->isVoidType())
return;
- if (DiagnoseNoDiscard(*this, cast_or_null<WarnUnusedResultAttr>(
- CE->getUnusedResultAttr(Context)),
- Loc, R1, R2, /*isCtor=*/false))
+ const auto &[OffendingDecl, A] = CE->getUnusedResultAttr(Context);
+ if (DiagnoseNoDiscard(*this, OffendingDecl,
+ cast_or_null<WarnUnusedResultAttr>(A), Loc, R1, R2,
+ /*isCtor=*/false))
return;
// If the callee has attribute pure, const, or warn_unused_result, warn with
@@ -313,16 +320,21 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S, unsigned DiagID) {
}
} else if (const auto *CE = dyn_cast<CXXConstructExpr>(E)) {
if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+ const NamedDecl *OffendingDecl = nullptr;
const auto *A = Ctor->getAttr<WarnUnusedResultAttr>();
- A = A ? A : Ctor->getParent()->getAttr<WarnUnusedResultAttr>();
- if (DiagnoseNoDiscard(*this, A, Loc, R1, R2, /*isCtor=*/true))
+ if (!A) {
+ OffendingDecl = Ctor->getParent();
+ A = OffendingDecl->getAttr<WarnUnusedResultAttr>();
+ }
+ if (DiagnoseNoDiscard(*this, OffendingDecl, A, Loc, R1, R2,
+ /*isCtor=*/true))
return;
}
} else if (const auto *ILE = dyn_cast<InitListExpr>(E)) {
if (const TagDecl *TD = ILE->getType()->getAsTagDecl()) {
- if (DiagnoseNoDiscard(*this, TD->getAttr<WarnUnusedResultAttr>(), Loc, R1,
- R2, /*isCtor=*/false))
+ if (DiagnoseNoDiscard(*this, TD, TD->getAttr<WarnUnusedResultAttr>(), Loc,
+ R1, R2, /*isCtor=*/false))
return;
}
} else if (ShouldSuppress)
@@ -336,8 +348,8 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S, unsigned DiagID) {
}
const ObjCMethodDecl *MD = ME->getMethodDecl();
if (MD) {
- if (DiagnoseNoDiscard(*this, MD->getAttr<WarnUnusedResultAttr>(), Loc, R1,
- R2, /*isCtor=*/false))
+ if (DiagnoseNoDiscard(*this, nullptr, MD->getAttr<WarnUnusedResultAttr>(),
+ Loc, R1, R2, /*isCtor=*/false))
return;
}
} else if (const PseudoObjectExpr *POE = dyn_cast<PseudoObjectExpr>(E)) {
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 693ca29370cf3f..da1f8201f55dcc 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
@@ -17,10 +17,10 @@ E get_e();
// cxx11-warning@-1 {{use of the 'nodiscard' attribute is a C++17 extension}}
void f() {
- get_s(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+ get_s(); // expected-warning {{ignoring return value of type 'S' declared with 'nodiscard' attribute}}
get_i(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
get_vi(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
- get_e(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+ get_e(); // expected-warning {{ignoring return value of type 'E' declared with 'nodiscard' attribute}}
// Okay, warnings are not encouraged
get_s_ref();
@@ -54,10 +54,10 @@ void f() {
fp3 three;
fp2_alias four;
- one(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
- two(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
- three(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
- four(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+ one(); // expected-warning {{ignoring return value of type 'E' declared with 'nodiscard' attribute}}
+ two(); // expected-warning {{ignoring return value of type 'S' declared with 'nodiscard' attribute}}
+ three(); // expected-warning {{ignoring return value of type 'S' declared with 'nodiscard' attribute}}
+ four(); // expected-warning {{ignoring return value of type 'S' declared with 'nodiscard' attribute}}
// These are all okay because of the explicit cast to void.
(void)one();
@@ -84,8 +84,8 @@ LaterReason get_later_reason();
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
void cxx20_use() {
- get_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: reason}}
- get_later_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: later reason}}
+ get_reason(); // expected-warning {{ignoring return value of type 'ReasonStruct' declared with 'nodiscard' attribute: reason}}
+ get_later_reason(); // expected-warning {{ignoring return value of type 'LaterReason' declared with 'nodiscard' attribute: later reason}}
another_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: another reason}}
conflicting_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: special reason}}
}
@@ -115,20 +115,20 @@ void usage() {
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);
- Y(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away either!}}
+ 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 function declared with 'nodiscard' attribute: Don't throw me away!}}
+ ConvertTo{}; // expected-warning {{ignoring return value of type 'ConvertTo' declared with 'nodiscard' attribute: Don't throw me away!}}
// AST is different in C++17 mode. Before, a move ctor for ConvertTo is there
// as well, hence the constructor warning.
- // since-cxx17-warning@+2 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
- // cxx11-warning@+1 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+ // since-cxx17-warning@+2 {{ignoring return value of type 'ConvertTo' declared with 'nodiscard' attribute: Don't throw me away!}}
+ // cxx11-warning@+1 {{ignoring temporary of type 'ConvertTo' declared with 'nodiscard' attribute: Don't throw me away!}}
(ConvertTo) s;
(int)s; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
(S)'c'; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
- // since-cxx17-warning@+2 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
- // cxx11-warning@+1 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
+ // since-cxx17-warning@+2 {{ignoring return value of type 'ConvertTo' declared with 'nodiscard' attribute: Don't throw me away!}}
+ // cxx11-warning@+1 {{ignoring temporary of type 'ConvertTo' declared with 'nodiscard' attribute: Don't throw me away!}}
static_cast<ConvertTo>(s);
static_cast<int>(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
static_cast<double>(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw away as a double}}
diff --git a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p3.cpp b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p3.cpp
index a3543cff7d2c92..b37517921b1ca4 100644
--- a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p3.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p3.cpp
@@ -8,7 +8,7 @@ namespace std_example {
error_info enable_missile_safety_mode();
void launch_missiles();
void test_missiles() {
- enable_missile_safety_mode(); // expected-warning {{ignoring return value of function declared with 'nodiscard'}}
+ enable_missile_safety_mode(); // expected-warning {{ignoring return value of type 'error_info' declared with 'nodiscard'}}
launch_missiles();
}
diff --git a/clang/test/Sema/c2x-nodiscard.c b/clang/test/Sema/c2x-nodiscard.c
index f8b0567366465d..e2537bcf1d29d6 100644
--- a/clang/test/Sema/c2x-nodiscard.c
+++ b/clang/test/Sema/c2x-nodiscard.c
@@ -31,10 +31,10 @@ enum E2 get_e(void);
[[nodiscard]] int get_i(void);
void f2(void) {
- get_s(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
- get_s3(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Wrong}}
+ get_s(); // expected-warning {{ignoring return value of type 'S4' declared with 'nodiscard' attribute}}
+ get_s3(); // expected-warning {{ignoring return value of type 'S3' declared with 'nodiscard' attribute: Wrong}}
get_i(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
- get_e(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+ get_e(); // expected-warning {{ignoring return value of type 'E2' declared with 'nodiscard' attribute}}
// Okay, warnings are not encouraged
(void)get_s();
@@ -50,7 +50,7 @@ struct [[nodiscard]] error_info{
struct error_info enable_missile_safety_mode(void);
void launch_missiles(void);
void test_missiles(void) {
- enable_missile_safety_mode(); // expected-warning {{ignoring return value of function declared with 'nodiscard'}}
+ enable_missile_safety_mode(); // expected-warning {{ignoring return value of type 'error_info' declared with 'nodiscard'}}
launch_missiles();
}
diff --git a/clang/test/SemaCXX/warn-unused-result.cpp b/clang/test/SemaCXX/warn-unused-result.cpp
index 4b7a2503ecc0dd..58c013d8478b00 100644
--- a/clang/test/SemaCXX/warn-unused-result.cpp
+++ b/clang/test/SemaCXX/warn-unused-result.cpp
@@ -108,7 +108,7 @@ void lazy() {
(void)DoAnotherThing();
(void)DoYetAnotherThing();
- DoSomething(); // expected-warning {{ignoring return value}}
+ DoSomething(); // expected-warning {{ignoring return value of type 'Status' declared with 'warn_unused_result'}}
DoSomethingElse();
DoAnotherThing();
DoYetAnotherThing();
@@ -120,11 +120,11 @@ class [[clang::warn_unused_result]] StatusOr {
StatusOr<int> doit();
void test() {
Foo f;
- f.doStuff(); // expected-warning {{ignoring return value}}
- doit(); // expected-warning {{ignoring return value}}
+ f.doStuff(); // expected-warning {{ignoring return value of type 'Status' declared with 'warn_unused_result'}}
+ doit(); // expected-warning {{ignoring return value of type 'StatusOr<int>' declared with 'warn_unused_result'}}
auto func = []() { return Status(); };
- func(); // expected-warning {{ignoring return value}}
+ func(); // expected-warning {{ignoring return value of type 'Status' declared with 'warn_unused_result'}}
}
}
@@ -139,7 +139,7 @@ struct Status {};
void Bar() {
Foo f;
- f.Bar(); // expected-warning {{ignoring return value}}
+ f.Bar(); // expected-warning {{ignoring return value of type 'Status' declared with 'warn_unused_result'}}
};
}
@@ -215,18 +215,18 @@ P operator--(const P &) { return {}; };
void f() {
S s;
P p;
- s.DoThing(); // expected-warning {{ignoring return value}}
- p.DoThing(); // expected-warning {{ignoring return value}}
+ s.DoThing(); // expected-warning {{ignoring return value of type 'S' declared with 'warn_unused_result'}}
+ p.DoThing(); // expected-warning {{ignoring return value of type 'P' declared with 'warn_unused_result'}}
// Only postfix is expected to warn when written correctly.
- s++; // expected-warning {{ignoring return value}}
- s--; // expected-warning {{ignoring return value}}
- p++; // expected-warning {{ignoring return value}}
- p--; // expected-warning {{ignoring return value}}
+ s++; // expected-warning {{ignoring return value of type 'S' declared with 'warn_unused_result'}}
+ s--; // expected-warning {{ignoring return value of type 'S' declared with 'warn_unused_result'}}
+ p++; // expected-warning {{ignoring return value of type 'P' declared with 'warn_unused_result'}}
+ p--; // expected-warning {{ignoring return value of type 'P' declared with 'warn_unused_result'}}
// Improperly written prefix operators should still warn.
- ++s; // expected-warning {{ignoring return value}}
- --s; // expected-warning {{ignoring return value}}
- ++p; // expected-warning {{ignoring return value}}
- --p; // expected-warning {{ignoring return value}}
+ ++s; // expected-warning {{ignoring return value of type 'S' declared with 'warn_unused_result'}}
+ --s; // expected-warning {{ignoring return value of type 'S' declared with 'warn_unused_result'}}
+ ++p; // expected-warning {{ignoring return value of type 'P' declared with 'warn_unused_result'}}
+ --p; // expected-warning {{ignoring return value of type 'P' declared with 'warn_unused_result'}}
// Silencing the warning by cast to void still works.
(void)s.DoThing();
@@ -243,7 +243,7 @@ namespace PR39837 {
void g() {
int a[2];
for (int b : a)
- f(b); // expected-warning {{ignoring return value}}
+ f(b); // expected-warning {{ignoring return value of function declared with 'warn_unused_result'}}
}
} // namespace PR39837
@@ -261,12 +261,12 @@ typedef a indirect;
a af1();
indirect indirectf1();
void af2() {
- af1(); // expected-warning {{ignoring return value}}
+ af1(); // expected-warning {{ignoring return value of type 'a' declared with 'warn_unused_result'}}
void *(*a1)();
a1(); // no warning
a (*a2)();
- a2(); // expected-warning {{ignoring return value}}
- indirectf1(); // expected-warning {{ignoring return value}}
+ a2(); // expected-warning {{ignoring return value of type 'a' declared with 'warn_unused_result'}}
+ indirectf1(); // expected-warning {{ignoring return value of type 'a' declared with 'warn_unused_result'}}
}
[[nodiscard]] typedef void *b1; // expected-warning {{'[[nodiscard]]' attribute ignored when applied to a typedef; consider using '__attribute__((warn_unused_result))' or '[[clang::warn_unused_result]]' instead}}
[[gnu::warn_unused_result]] typedef void *b2; // expected-warning {{'[[gnu::warn_unused_result]]' attribute ignored when applied to a typedef; consider using '__attribute__((warn_unused_result))' or '[[clang::warn_unused_result]]' instead}}
@@ -279,10 +279,79 @@ void bf2() {
__attribute__((warn_unused_result)) typedef void *c;
c cf1();
void cf2() {
- cf1(); // expected-warning {{ignoring return value}}
+ cf1(); // expected-warning {{ignoring return value of type 'c' declared with 'warn_unused_result'}}
void *(*c1)();
c1();
c (*c2)();
- c2(); // expected-warning {{ignoring return value}}
+ c2(); // expected-warning {{ignoring return value of type 'c' declared with 'warn_unused_result'}}
}
}
+
+namespace nodiscard_specialization {
+// Test to only mark a specialization of class template as nodiscard
+template<typename T> struct S { S(int) {} };
+template<> struct [[nodiscard...
[truncated]
|
|
CC @Sirraide @erichkeane. (Sorry, I do not have permission to request reviewers, so instead used ping) |
Mick235711
force-pushed
the
improve-nodiscard
branch
3 times, most recently
from
e69fcc0 to
35be448
Compare
Mick235711
force-pushed
the
improve-nodiscard
branch
from
35be448 to
59f7dbd
Compare
|
Release note added. Incidentally, this also fixes an inconsistency for Clang compared to GCC/MSVC: In the following program struct [[nodiscard("Reason 1")]] S {};
[[nodiscard("Reason 2")]] S getS();
int main()
{
getS();
}My opinion is that the function's attribute should take precedence since it is "more specific". GCC/MSVC trunk agrees with me and prints Reason 2 in warning, while Clang trunk prints Reason 1. (Compiler Explorer). After this PR, Clang will also print Reason 2 here. Also, the same CE link also explores the note generation on |
That sounds reasonable, yeah. |
There was a problem hiding this comment.
I think this is fine the way it is, but I’ll leave it to @erichkeane to approve this seeing as I proposed this approach here iirc.
Side note: please try to avoid force-pushing in the future as that makes it really annoying to try and figure out what has changed since the last review. We always squash on merge anyway, so a clean commit history on a pr is irrelevant.
Especially if the last review was a week ago ;Þ |
Oh, sorry about that, I don't usually force push, but I assumed that LLVM PRs should be one-commit only when I see a similarly-worded line in the contributing guidelines. My bad. |
I'm away at LLVM dev right now. So won't get a chance to review until next week, but I promise this is one I'll look at asap |
|
Gentle ping @erichkeane |
|
Fyi, Erich is unavailable again until later this month, so you’ll have to wait a bit longer unfortunately... |
|
@Mick235711 Congratulations on having your first Pull Request (PR) merged into the LLVM Project! Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR. Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues. How to do this, and the rest of the post-merge process, is covered in detail here. If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again. If you don't get any reports, no action is required from you. Your changes are working as expected, well done! |
revert: breaks build of hipRuntime, need hipRuntime fix 51ad290 [Clang] Improve diagnostic on `[[nodiscard]]` attribute (llvm#112521) Change-Id: I316909ee244ab7482d13add3a8b938af2187031e
|
Looks like this caused a regression: #117975 |
A follow-up to llvm#112289. When diagnosing an unused return value, if the diagnostic is triggered by an attribute attached to a type, the type name is now included in the diagnostic. --------- Co-authored-by: Sirraide <[email protected]> Change-Id: I03f6ee4fde4b9df42e5b3b56881d28ccaeccb5b7
llvm#118636) Fixes llvm#117975, a regression introduced by llvm#112521 due to forgetting to check for `nullptr` before dereferencing in `CallExpr::getUnusedResultAttr`.
A follow-up to llvm#112289. When diagnosing an unused return value, if the diagnostic is triggered by an attribute attached to a type, the type name is now included in the diagnostic. --------- Co-authored-by: Sirraide <[email protected]>
A follow-up and alternative proposal to #112289.
In this PR, no new notes are added to avoid noise. Instead, originally, every
[[nodiscard]]trigger, regardless of whether it is on function declaration or return type, has the same warning. For instance, for the following program:The generated warning currently is the same for two calls in
main(): (Compiler Explorer)As suggested by #112289 (comment), this PR thus makes a distinction here by making
[[nodiscard]]that are triggered by its placement on return types specifically points out what the return type is:In addition to better clarity, this also helps identify which specialization is marked as
[[nodiscard]], as the standard allows[[nodiscard]]to be placed on some specializations of a class template only, as demonstrated by #112289 (comment).For constructor calls, a different warning message is also added. Currently, for
[[nodiscard]](but not for__attribute__((warn_unused_result))), temporaries that are discarded as a result of a constructor call generates a different warning:warning: ignoring temporary created by a constructor declared with 'nodiscard' attributeAfter this PR, if
[[nodiscard]]is placed on the constructor itself, nothing changed. If[[nodiscard]]is placed on the type, the new warning is:("created by a constructor" is removed since "declared with 'nodiscard'" applies to the type, not the constructor)
One thing to note here is that for the following scenario:
The warning message is generated in the format for constructor placement ("ignoring temporary created by a constructor"). Similarly, for
[[nodiscard]]/warn_unused_resulton both functions and its return type, the function attribute takes precedence as that is probably "more specific".Compared to the original PR,
pureandconstare not touched, so there are no builtin-related issues this time.Some new test cases are added at the end of
SemaCXX/warn-unused-result.cppto test the new warning.