[clang] Generate note on declaration for nodiscard-related attributes #112289
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) ChangesCurrently, the following program [[nodiscard]] int fun() { return 1; }
[[deprecated]] int fun2() { return 2; }
int main()
{
fun(); fun2();
}generates the following diagnostics on Clang trunk: (Compiler Explorer) <source>:6:5: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
6 | fun(); fun2();
| ^~~
<source>:6:12: warning: 'fun2' is deprecated [-Wdeprecated-declarations]
6 | fun(); fun2();
| ^
<source>:2:3: note: 'fun2' has been explicitly marked deprecated here
2 | [[deprecated]] int fun2() { return 2; }
| ^
2 warnings generated.There seems to exist a discrepancy between This PR tries to fix this discrepancy by additionally generating a note on the declaration of all test.cpp:6:5: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
6 | fun(); fun2();
| ^~~
test.cpp:1:3: note: 'fun' has been explicitly marked nodiscard here
1 | [[nodiscard]] int fun() { return 1; }
| ^
test.cpp:6:12: warning: 'fun2' is deprecated [-Wdeprecated-declarations]
6 | fun(); fun2();
| ^
test.cpp:2:3: note: 'fun2' has been explicitly marked deprecated here
2 | [[deprecated]] int fun2() { return 2; }
| ^
2 warnings generated.FIrst time contributor here who is not very familiar with Clang's infrastructure... Any comments and suggestions are welcome. No new test cases are added, as I felt that adding Patch is 23.10 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/112289.diff 12 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..3f6e4c35146454 100644
--- a/clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -9290,6 +9290,8 @@ def warn_unused_result_typedef_unsupported_spelling : Warning<
def warn_unused_volatile : Warning<
"expression result unused; assign into a variable to force a volatile load">,
InGroup<DiagGroup<"unused-volatile-lvalue">>;
+def note_nodiscard_specified_here : Note<
+ "%0 has been explicitly marked %1 here">;
def ext_cxx14_attr : Extension<
"use of the %0 attribute is a C++14 extension">, InGroup<CXX14Attrs>;
diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
index 9ecbf121e3fc0d..b9450d6a61e57c 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 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;
+ return {TD->getDecl(), 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 {dyn_cast<NamedDecl>(D),
+ D ? D->getAttr<WarnUnusedResultAttr>() : nullptr};
}
SourceLocation CallExpr::getBeginLoc() const {
diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp
index 9e235a46707cd4..baaa6422b88fd4 100644
--- a/clang/lib/Sema/SemaStmt.cpp
+++ b/clang/lib/Sema/SemaStmt.cpp
@@ -14,6 +14,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/ASTDiagnostic.h"
#include "clang/AST/ASTLambda.h"
+#include "clang/AST/Attrs.inc"
#include "clang/AST/CXXInheritance.h"
#include "clang/AST/CharUnits.h"
#include "clang/AST/DeclObjC.h"
@@ -204,23 +205,29 @@ 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();
+ bool result;
if (Msg.empty()) {
if (IsCtor)
- return S.Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
- return S.Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
- }
+ result = S.Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
+ else
+ result = S.Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
+ } else if (IsCtor)
+ result = S.Diag(Loc, diag::warn_unused_constructor_msg)
+ << A << Msg << R1 << R2;
+ else
+ result = S.Diag(Loc, diag::warn_unused_result_msg) << A << 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_result_msg) << A << Msg << R1 << R2;
+ if (OffendingDecl)
+ S.Diag(A->getLocation(), diag::note_nodiscard_specified_here)
+ << OffendingDecl << A->getSpelling();
+ return result;
}
void Sema::DiagnoseUnusedExprResult(const Stmt *S, unsigned DiagID) {
@@ -290,9 +297,12 @@ 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 NamedDecl *OffendingDecl;
+ const Attr *A;
+ std::tie(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
@@ -302,27 +312,38 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S, unsigned DiagID) {
if (const Decl *FD = CE->getCalleeDecl()) {
if (ShouldSuppress)
return;
- if (FD->hasAttr<PureAttr>()) {
+ if (const auto *A = FD->getAttr<PureAttr>()) {
Diag(Loc, diag::warn_unused_call) << R1 << R2 << "pure";
+ if (OffendingDecl && !OffendingDecl->getIdentifier()->getBuiltinID())
+ Diag(A->getLocation(), diag::note_nodiscard_specified_here)
+ << OffendingDecl << "pure";
return;
}
- if (FD->hasAttr<ConstAttr>()) {
+ if (const auto *A = FD->getAttr<ConstAttr>()) {
Diag(Loc, diag::warn_unused_call) << R1 << R2 << "const";
+ if (OffendingDecl && !OffendingDecl->getIdentifier()->getBuiltinID())
+ Diag(A->getLocation(), diag::note_nodiscard_specified_here)
+ << OffendingDecl << "const";
return;
}
}
} else if (const auto *CE = dyn_cast<CXXConstructExpr>(E)) {
if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
+ const NamedDecl *OffendingDecl = Ctor;
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 +357,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, MD, 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..158fbdbd67be34 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
@@ -2,18 +2,18 @@
// RUN: %clang_cc1 -fsyntax-only -std=c++17 -verify=expected,cxx11-17,since-cxx17 -pedantic %s
// RUN: %clang_cc1 -fsyntax-only -std=c++20 -verify=expected,since-cxx17 -pedantic %s
-struct [[nodiscard]] S {};
+struct [[nodiscard]] S {}; // expected-note 4 {{'S' has been explicitly marked nodiscard here}}
// cxx11-warning@-1 {{use of the 'nodiscard' attribute is a C++17 extension}}
S get_s();
S& get_s_ref();
-enum [[nodiscard]] E {};
+enum [[nodiscard]] E {}; // expected-note 2 {{'E' has been explicitly marked nodiscard here}}
// cxx11-warning@-1 {{use of the 'nodiscard' attribute is a C++17 extension}}
E get_e();
-[[nodiscard]] int get_i();
+[[nodiscard]] int get_i(); // expected-note {{'get_i' has been explicitly marked nodiscard here}}
// cxx11-warning@-1 {{use of the 'nodiscard' attribute is a C++17 extension}}
-[[nodiscard]] volatile int &get_vi();
+[[nodiscard]] volatile int &get_vi(); // expected-note {{'get_vi' has been explicitly marked nodiscard here}}
// cxx11-warning@-1 {{use of the 'nodiscard' attribute is a C++17 extension}}
void f() {
@@ -32,6 +32,7 @@ void f() {
[[nodiscard]] volatile char &(*fp)(); // expected-warning {{'nodiscard' attribute only applies to functions, classes, or enumerations}}
// cxx11-warning@-1 {{use of the 'nodiscard' attribute is a C++17 extension}}
+// expected-note@-2 {{'fp' has been explicitly marked nodiscard here}}
void g() {
fp(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
@@ -67,20 +68,20 @@ void f() {
}
} // namespace PR31526
-struct [[nodiscard("reason")]] ReasonStruct {};
+struct [[nodiscard("reason")]] ReasonStruct {}; // expected-note {{'ReasonStruct' has been explicitly marked nodiscard here}}
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
struct LaterReason;
-struct [[nodiscard("later reason")]] LaterReason {};
+struct [[nodiscard("later reason")]] LaterReason {}; // expected-note {{'LaterReason' has been explicitly marked nodiscard here}}
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
ReasonStruct get_reason();
LaterReason get_later_reason();
-[[nodiscard("another reason")]] int another_reason();
+[[nodiscard("another reason")]] int another_reason(); // expected-note {{'another_reason' has been explicitly marked nodiscard here}}
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
[[nodiscard("conflicting reason")]] int conflicting_reason();
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
-[[nodiscard("special reason")]] int conflicting_reason();
+[[nodiscard("special reason")]] int conflicting_reason(); // expected-note {{'conflicting_reason' has been explicitly marked nodiscard here}}
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
void cxx20_use() {
@@ -91,23 +92,23 @@ void cxx20_use() {
}
namespace p1771 {
-struct[[nodiscard("Don't throw me away!")]] ConvertTo{};
+struct[[nodiscard("Don't throw me away!")]] ConvertTo{}; // expected-note 3 {{'ConvertTo' has been explicitly marked nodiscard here}}
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
struct S {
- [[nodiscard]] S();
+ [[nodiscard]] S(); // expected-note {{'S' has been explicitly marked nodiscard here}}
// cxx11-warning@-1 {{use of the 'nodiscard' attribute is a C++17 extension}}
- [[nodiscard("Don't let that S-Char go!")]] S(char);
+ [[nodiscard("Don't let that S-Char go!")]] S(char); // expected-note 2 {{'S' has been explicitly marked nodiscard here}}
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
S(int);
[[gnu::warn_unused_result]] S(double);
operator ConvertTo();
- [[nodiscard]] operator int();
+ [[nodiscard]] operator int(); // expected-note 2 {{'operator int' has been explicitly marked nodiscard here}}
// cxx11-warning@-1 {{use of the 'nodiscard' attribute is a C++17 extension}}
- [[nodiscard("Don't throw away as a double")]] operator double();
+ [[nodiscard("Don't throw away as a double")]] operator double(); // expected-note {{'operator double' has been explicitly marked nodiscard here}}
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
};
-struct[[nodiscard("Don't throw me away either!")]] Y{};
+struct[[nodiscard("Don't throw me away either!")]] Y{}; // expected-note {{'Y' has been explicitly marked nodiscard here}}
// cxx11-17-warning@-1 {{use of the 'nodiscard' attribute is a C++20 extension}}
void usage() {
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..6df334e1c0b488 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
@@ -1,7 +1,7 @@
// RUN: %clang_cc1 -std=c++1z -verify %s
namespace std_example {
- struct [[nodiscard]] error_info{
+ struct [[nodiscard]] error_info{ // expected-note {{'error_info' has been explicitly marked nodiscard here}}
// ...
};
diff --git a/clang/test/OpenMP/declare_variant_messages.cpp b/clang/test/OpenMP/declare_variant_messages.cpp
index b8a806e7ef75b3..5fe53d70270fbc 100644
--- a/clang/test/OpenMP/declare_variant_messages.cpp
+++ b/clang/test/OpenMP/declare_variant_messages.cpp
@@ -133,7 +133,7 @@ int score_and_cond_const();
template <int C>
int score_and_cond_const_inst();
-__attribute__((pure)) int pure() { return 0; }
+__attribute__((pure)) int pure() { return 0; } // expected-note {{'pure' has been explicitly marked pure here}}
#pragma omp declare variant(pure) match(user = {condition(1)}) // expected-warning {{ignoring return value of function declared with pure attribute}}
int unused_warning_after_specialization() { return foo(); }
diff --git a/clang/test/Sema/c2x-nodiscard.c b/clang/test/Sema/c2x-nodiscard.c
index f8b0567366465d..8865e2f9cd0ca2 100644
--- a/clang/test/Sema/c2x-nodiscard.c
+++ b/clang/test/Sema/c2x-nodiscard.c
@@ -9,7 +9,7 @@ struct [[nodiscard]] S1 { // ok
struct [[nodiscard, nodiscard]] S2 { // ok
int i;
};
-struct [[nodiscard("Wrong")]] S3 {
+struct [[nodiscard("Wrong")]] S3 { // expected-note {{'S3' has been explicitly marked nodiscard here}}
int i;
};
@@ -20,15 +20,15 @@ enum [[nodiscard]] E1 { One };
[[nodiscard]] int i; // expected-warning {{'nodiscard' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
-struct [[nodiscard]] S4 {
+struct [[nodiscard]] S4 { // expected-note {{'S4' has been explicitly marked nodiscard here}}
int i;
};
struct S4 get_s(void);
-enum [[nodiscard]] E2 { Two };
+enum [[nodiscard]] E2 { Two }; // expected-note {{'E2' has been explicitly marked nodiscard here}}
enum E2 get_e(void);
-[[nodiscard]] int get_i(void);
+[[nodiscard]] int get_i(void); // expected-note {{'get_i' has been explicitly marked nodiscard here}}
void f2(void) {
get_s(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
@@ -43,7 +43,7 @@ void f2(void) {
(void)get_e();
}
-struct [[nodiscard]] error_info{
+struct [[nodiscard]] error_info{ // expected-note {{'error_info' has been explicitly marked nodiscard here}}
int i;
};
@@ -54,7 +54,7 @@ void test_missiles(void) {
launch_missiles();
}
-[[nodiscard]] int f3();
+[[nodiscard]] int f3(); // expected-note {{'f3' has been explicitly marked nodiscard here}}
void GH104391() {
#define M (unsigned int) f3()
diff --git a/clang/test/Sema/unused-expr.c b/clang/test/Sema/unused-expr.c
index 6723a33cbd4e01..9e444f94d67137 100644
--- a/clang/test/Sema/unused-expr.c
+++ b/clang/test/Sema/unused-expr.c
@@ -73,15 +73,15 @@ void t4(int a) {
for (;;b < 1) {} // expected-warning{{relational comparison result unused}}
}
-int t5f(void) __attribute__((warn_unused_result));
+int t5f(void) __attribute__((warn_unused_result)); // expected-note {{'t5f' has been explicitly marked warn_unused_result here}}
void t5(void) {
t5f(); // expected-warning {{ignoring return value of function declared with 'warn_unused_result' attribute}}
}
-int fn1(void) __attribute__ ((warn_unused_result));
-int fn2() __attribute__ ((pure));
-int fn3() __attribute__ ((__const));
+int fn1(void) __attribute__ ((warn_unused_result)); // expected-note 3 {{'fn1' has been explicitly marked warn_unused_result here}}
+int fn2() __attribute__ ((pure)); // expected-note 2 {{'fn2' has been explicitly marked pure here}}
+int fn3() __attribute__ ((__const)); // expected-note {{'fn3' has been explicitly marked const here}}
int t6(void) {
if (fn1() < 0 || fn2(2,1) < 0 || fn3(2) < 0) // no warnings
return -1;
@@ -97,7 +97,7 @@ int t6(void) {
int t7 __attribute__ ((warn_unused_result)); // expected-warning {{'warn_unused_result' attribute only applies to Objective-C methods, enums, structs, unions, classes, functions, function pointers, and typedefs}}
// PR4010
-int (*fn4)(void) __attribute__ ((warn_unused_result));
+int (*fn4)(void) __attribute__ ((warn_unused_result)); // expected-note {{'fn4' has been explicitly marked warn_unused_result here}}
void t8(void) {
fn4(); // expected-warning {{ignoring return value of function declared with 'warn_unused_result' attribute}}
}
diff --git a/clang/test/SemaCXX/coroutines.cpp b/clang/test/SemaCXX/coroutines.cpp
index 068fdab4bfe384..c9d442a0338121 100644
--- a/clang/test/SemaCXX/coroutines.cpp
+++ b/clang/test/SemaCXX/coroutines.cpp
@@ -1497,7 +1497,7 @@ class awaitable_unused_warn {
using handle_type = std::coroutine_handle<>;
constexpr bool await_ready() noexcept { return false; }
void await_suspend(handle_type) noexcept {}
- [[nodiscard]] int await_resume() noexcept { return 1; }
+ [[nodiscard]] int await_resume() noexcept { return 1; } // expected-note 2 {{'await_resume' has been explicitly marked nodiscard here}}
};
template <class Await>
diff --git a/clang/test/SemaCXX/warn-unused-result.cpp b/clang/test/SemaCXX/warn-unused-result.cpp
index 4b7a2503ecc0dd..247be530274af2 100644
--- a/clang/test/SemaCXX/warn-unused-result.cpp
+++ b/clang/test/SemaCXX/warn-unused-result.cpp
@@ -1,13 +1,13 @@
// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
-int f() __attribute__((warn_unused_result));
+int f() __attribute__((warn_unused_result)); // expected-note 12 {{'f' has been explicitly marked warn_unused_result here}}
struct S {
void t() const;
};
-S g1() __attribute__((warn_unused_result));
-S *g2() __attribute__((warn_unused_result));
-S &g3() __attribute__((warn_unused_result));
+S g1() __attribute__((warn_unused_result)); // expected-note {{'g1' has been explicitly marked warn_unused_result here}}
+S *g2() __attribute__((warn_unused_result)); // expected-note {{'g2' has been explicitly marked warn_unused_result here}}
+S &g3() __attribute__((warn_unused_result)); // expected-note {{'g3' has been explicitly marked warn_unused_result here}}
void test() {
f(); // expected-warning {{ignoring return value}}
@@ -64,7 +64,7 @@ void testSubstmts(int i) {
}
struct X {
- int foo() __attribute__((warn_unused_result));
+ int foo() __attribute__((warn_unused_result)); // expected-note 2 {{'foo' has been explicitly marked warn_unused_result here}}
};
void bah() {
@@ -80,7 +80,7 @@ class Foo {
Status doStuff();
};
-struct [[clang::warn_unused_result]] Status {
+struct [[clang::warn_unused_result]] Status { // expected-note 3 {{'Status' has been explicitly marked warn_unused_result here}}
bool ok() const;
Status& operator=(const Status& x);
inline void Update(const Status& new_status) {
@@ -115,7 +115,7 @@ void lazy() {
}
template ...
[truncated]
|
There was a problem hiding this comment.
Hmm, I can’t really think of a situation when this note would actually help... at least for deprecated, you could maybe argue that you might want to know what part of the codebase marked it as deprecated in case there are multiple declarations, but for nodiscard, I don’t think you really care that much, so I feel like this might just be adding noise instead of helping. Imo just the error is enough, but that’s just my opinion.
As I said, I feel like that might be by design, though.
Yeah, there are already enough tests for this. |
Ah, 1 more case that comes to mind for |
The one use case I could see is that the So this is definitely causing me troubles with 'motivation'. |
I think there is still some merits for these kind of overload sets. For instance, API like std::span<CharT> span() const noexcept;
void span( std::span<CharT> s ) noexcept;In these kind of cases the first overload can be marked [[nodiscard]], which is probably reasonable.
Indeed, I'm currently torn about this. On the one hand, the same argument can be applied to nodiscard, where you might want to know what part of the codebase marked it as nodiscard in case of multiple declarations. On the other hand, the motivation is definitely weaker here.
I can suggest one motivation here, which potentially applies to [[nodiscard]] on types. In the standard, it is allowed to mark only some specializations of a class template as nodiscard: template<typename T> struct S {};
template<> struct [[nodiscard]] S<int> {};
template<typename T>
S<T> getS(const T&) { return {}; }
int main()
{
getS(2.0); // no warn
getS(2); // warns
}In this case, the current warning is not especially good, the only thing mentioned is: (Compiler Explorer) <source>:10:5: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
10 | getS(2);
| ^~~~ ~
1 warning generated.Nowhere is the return type test-2.cpp:10:5: warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]
10 | getS(2);
| ^~~~ ~
test-2.cpp:2:21: note: 'S<int>' has been explicitly marked nodiscard here
2 | template<> struct [[nodiscard]] S<int> {};
| ^
1 warning generated.Here the specialization is named, which is potentially helpful. Although I'll admit this is a bit of a contrived example. The noise argument does makes sense though, so I'm currently undecided about this too. |
Hmm, the diagnostic for the error here isn’t particularly great to begin with; I feel like it should instead mention that the return type, not the function, is marked |
|
I.e. could we get it to emit something like: <source>:10:5: warning: ignoring return value of type 'S<int>' declared with 'nodiscard' attribute [-Wunused-result]
10 | getS(2);
| ^~~~ ~
1 warning generated.That is, keep the current wording if the function is nodiscard, but change it to mention the type instead if the type is marked nodiscard—provided there is a relatively straight-forward way of doing this. |
I think this should be okay-ish to implement since we basically already have that information when implementing the note anyway. Will try to implement this tomorrow, if I can find some time... |
Mick235711
force-pushed
the
nodiscard-note
branch
from
05e44dc to
2376ab3
Compare
| StringRef type = (isa<PureAttr>(A) ? "pure" : "const"); | ||
| Diag(Loc, diag::warn_unused_call) << R1 << R2 << type; | ||
| if (const auto *ND = dyn_cast<NamedDecl>(OffendingDecl)) { | ||
| if (!ND->getIdentifier()->getBuiltinID()) |
There was a problem hiding this comment.
I still didn't get the justification for skipping if this is a builtin? It seems sensible to do so, even if the source location is a builtin.
There was a problem hiding this comment.
As I said above, the real motivation for avoiding marking here is twofold:
- Builtins like
isdigitare just marked pure internally, the note "explicitly marked pure/const here" is just wrong, there is no such attribute on that line.
int isdigit(int c) __attribute__((overloadable)); // The note actually fires on this line, which doesn't have a pure attribute
int isdigit(int c) __attribute__((overloadable)) // expected-note {{'isdigit' has been explicitly marked unavailable here}}
__attribute__((enable_if(c <= -1 || c > 255, "'c' must have the value of an unsigned char or EOF")))
__attribute__((unavailable("'c' must have the value of an unsigned char or EOF")));
void test3(int c) {
isdigit(c); // expected-warning{{ignoring return value of function declared with pure attribute}}
isdigit(10); // expected-warning{{ignoring return value of function declared with pure attribute}}
#ifndef CODEGEN
isdigit(-10); // expected-error{{'isdigit' is unavailable: 'c' must have the value of an unsigned char or EOF}}
#endif
}Unless A->getLocation() is not the right invocation to find the attribute location, I don't think there is a "builtin" declaration line that can be referenced by the note.
- Language builtins like
__builtin_operator_newwill just fire the note on the first use:
void test_typo_in_args() {
__builtin_operator_new(DNE); // expected-error {{undeclared identifier 'DNE'}}
__builtin_operator_new(DNE, DNE2); // expected-error {{undeclared identifier 'DNE'}} expected-error {{'DNE2'}}
__builtin_operator_delete(DNE); // expected-error {{'DNE'}}
__builtin_operator_delete(DNE, DNE2); // expected-error {{'DNE'}} expected-error {{'DNE2'}}
}In this test case (SemaCXX/builtin-operator-new-delete.cpp), the note is generated on the second line, i.e. first use of builtin, which is even more wrong as it is not even a declaration.
Excluding builtins from note generation fixes both errors, I think...
There was a problem hiding this comment.
Hmm... thats actually pretty strange: https://godbolt.org/z/q6EMMs6fa
If you make it NOT an error for the first declaration, it does what I suspect it SHOULD be doing in all cases, creating the overload set without a location: https://godbolt.org/z/4zeq99bne
So there is perhaps a bug in the builtin recovery we need to understand before we can move on.
Mick235711
force-pushed
the
nodiscard-note
branch
from
2376ab3 to
aaad12b
Compare
Mick235711
force-pushed
the
nodiscard-note
branch
from
aaad12b to
b9911d5
Compare
A follow-up to #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]>
|
Closed in favor of #112521. |
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
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]>
Currently, the following program
generates the following diagnostics on Clang trunk: (Compiler Explorer)
There seems to exist a discrepancy between
[[deprecated]]and[[nodiscard]]. The former generates a warning on the usage of the function, together with a note on the declaration of the function. In contrast, the latter only generates a warning.This PR tries to fix this discrepancy by additionally generating a note on the declaration of all
[[nodiscard]]-related attributes (nodiscard,warn_unused_result,pure, andconst). All 4 attributes generate a warning on ignoring the return value/constructor result on the trunk, and after this PR, all 4 attributes additionally generate a note. The above program will output the following diagnostics after this PR:FIrst time contributor here who is not very familiar with Clang's infrastructure... Any comments and suggestions are welcome. No new test cases are added, as I felt that adding
expected-notes on existing test cases already serves as testing for this PR.