Skip to content

Commit 46441fd

Browse files
author
Erich Keane
committedJul 25, 2019
Implement P1771
As passed in the Cologne meeting and treated by Core as a DR, [[nodiscard]] was applied to constructors so that they can be diagnosed in cases where the user forgets a variable name for a type. The intent is to enable the library to start using this on the constructors of scope_guard/lock_guard. Differential Revision: https://reviews.llvm.org/D64914 llvm-svn: 367027
1 parent 207726c commit 46441fd

File tree

9 files changed

+158
-20
lines changed

9 files changed

+158
-20
lines changed
 

‎clang/include/clang/Basic/Attr.td

+8-1
Original file line numberDiff line numberDiff line change
@@ -2335,12 +2335,19 @@ def WarnUnused : InheritableAttr {
23352335
}
23362336

23372337
def WarnUnusedResult : InheritableAttr {
2338-
let Spellings = [CXX11<"", "nodiscard", 201603>, C2x<"", "nodiscard">,
2338+
let Spellings = [CXX11<"", "nodiscard", 201907>, C2x<"", "nodiscard">,
23392339
CXX11<"clang", "warn_unused_result">,
23402340
GCC<"warn_unused_result">];
23412341
let Subjects = SubjectList<[ObjCMethod, Enum, Record, FunctionLike]>;
23422342
let Args = [StringArgument<"Message", 1>];
23432343
let Documentation = [WarnUnusedResultsDocs];
2344+
let AdditionalMembers = [{
2345+
// Check whether this the C++11 nodiscard version, even in non C++11
2346+
// spellings.
2347+
bool IsCXX11NoDiscard() const {
2348+
return this->getSemanticSpelling() == CXX11_nodiscard;
2349+
}
2350+
}];
23442351
}
23452352

23462353
def Weak : InheritableAttr {

‎clang/include/clang/Basic/AttrDocs.td

+27
Original file line numberDiff line numberDiff line change
@@ -1500,6 +1500,33 @@ in any resulting diagnostics.
15001500
}
15011501
error_info &foo();
15021502
void f() { foo(); } // Does not diagnose, error_info is a reference.
1503+
1504+
Additionally, discarded temporaries resulting from a call to a constructor
1505+
marked with ``[[nodiscard]]`` or a constructor of a type marked
1506+
``[[nodiscard]]`` will also diagnose. This also applies to type conversions that
1507+
use the annotated ``[[nodiscard]]`` constructor or result in an annotated type.
1508+
1509+
.. code-block: c++
1510+
struct [[nodiscard]] marked_type {/*..*/ };
1511+
struct marked_ctor {
1512+
[[nodiscard]] marked_ctor();
1513+
marked_ctor(int);
1514+
};
1515+
1516+
struct S {
1517+
operator marked_type() const;
1518+
[[nodiscard]] operator int() const;
1519+
};
1520+
1521+
void usages() {
1522+
marked_type(); // diagnoses.
1523+
marked_ctor(); // diagnoses.
1524+
marked_ctor(3); // Does not diagnose, int constructor isn't marked nodiscard.
1525+
1526+
S s;
1527+
static_cast<marked_type>(s); // diagnoses
1528+
(int)s; // diagnoses
1529+
}
15031530
}];
15041531
}
15051532

‎clang/include/clang/Basic/DiagnosticSemaKinds.td

+6
Original file line numberDiff line numberDiff line change
@@ -7430,6 +7430,12 @@ def warn_unused_container_subscript_expr : Warning<
74307430
def warn_unused_call : Warning<
74317431
"ignoring return value of function declared with %0 attribute">,
74327432
InGroup<UnusedValue>;
7433+
def warn_unused_constructor : Warning<
7434+
"ignoring temporary created by a constructor declared with %0 attribute">,
7435+
InGroup<UnusedValue>;
7436+
def warn_unused_constructor_msg : Warning<
7437+
"ignoring temporary created by a constructor declared with %0 attribute: %1">,
7438+
InGroup<UnusedValue>;
74337439
def warn_side_effects_unevaluated_context : Warning<
74347440
"expression with side effects has no effect in an unevaluated context">,
74357441
InGroup<UnevaluatedExpression>;

‎clang/lib/AST/Expr.cpp

+19-1
Original file line numberDiff line numberDiff line change
@@ -2563,13 +2563,31 @@ bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
25632563
case CXXTemporaryObjectExprClass:
25642564
case CXXConstructExprClass: {
25652565
if (const CXXRecordDecl *Type = getType()->getAsCXXRecordDecl()) {
2566-
if (Type->hasAttr<WarnUnusedAttr>()) {
2566+
const auto *WarnURAttr = Type->getAttr<WarnUnusedResultAttr>();
2567+
if (Type->hasAttr<WarnUnusedAttr>() ||
2568+
(WarnURAttr && WarnURAttr->IsCXX11NoDiscard())) {
25672569
WarnE = this;
25682570
Loc = getBeginLoc();
25692571
R1 = getSourceRange();
25702572
return true;
25712573
}
25722574
}
2575+
2576+
const auto *CE = cast<CXXConstructExpr>(this);
2577+
if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
2578+
const auto *WarnURAttr = Ctor->getAttr<WarnUnusedResultAttr>();
2579+
if (WarnURAttr && WarnURAttr->IsCXX11NoDiscard()) {
2580+
WarnE = this;
2581+
Loc = getBeginLoc();
2582+
R1 = getSourceRange();
2583+
2584+
if (unsigned NumArgs = CE->getNumArgs())
2585+
R2 = SourceRange(CE->getArg(0)->getBeginLoc(),
2586+
CE->getArg(NumArgs - 1)->getEndLoc());
2587+
return true;
2588+
}
2589+
}
2590+
25732591
return false;
25742592
}
25752593

‎clang/lib/Sema/SemaDeclAttr.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -2831,7 +2831,8 @@ static void handleSentinelAttr(Sema &S, Decl *D, const ParsedAttr &AL) {
28312831

28322832
static void handleWarnUnusedResult(Sema &S, Decl *D, const ParsedAttr &AL) {
28332833
if (D->getFunctionType() &&
2834-
D->getFunctionType()->getReturnType()->isVoidType()) {
2834+
D->getFunctionType()->getReturnType()->isVoidType() &&
2835+
!isa<CXXConstructorDecl>(D)) {
28352836
S.Diag(AL.getLoc(), diag::warn_attribute_void_function_method) << AL << 0;
28362837
return;
28372838
}

‎clang/lib/Sema/SemaStmt.cpp

+44-15
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,25 @@ static bool DiagnoseUnusedComparison(Sema &S, const Expr *E) {
196196
return true;
197197
}
198198

199+
static bool DiagnoseNoDiscard(Sema &S, const WarnUnusedResultAttr *A,
200+
SourceLocation Loc, SourceRange R1,
201+
SourceRange R2, bool IsCtor) {
202+
if (!A)
203+
return false;
204+
StringRef Msg = A->getMessage();
205+
206+
if (Msg.empty()) {
207+
if (IsCtor)
208+
return S.Diag(Loc, diag::warn_unused_constructor) << A << R1 << R2;
209+
return S.Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
210+
}
211+
212+
if (IsCtor)
213+
return S.Diag(Loc, diag::warn_unused_constructor_msg) << A << Msg << R1
214+
<< R2;
215+
return S.Diag(Loc, diag::warn_unused_result_msg) << A << Msg << R1 << R2;
216+
}
217+
199218
void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
200219
if (const LabelStmt *Label = dyn_cast_or_null<LabelStmt>(S))
201220
return DiagnoseUnusedExprResult(Label->getSubStmt());
@@ -254,19 +273,19 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
254273
return;
255274

256275
E = WarnExpr;
276+
if (const auto *Cast = dyn_cast<CastExpr>(E))
277+
if (Cast->getCastKind() == CK_NoOp ||
278+
Cast->getCastKind() == CK_ConstructorConversion)
279+
E = Cast->getSubExpr()->IgnoreImpCasts();
280+
257281
if (const CallExpr *CE = dyn_cast<CallExpr>(E)) {
258282
if (E->getType()->isVoidType())
259283
return;
260284

261-
if (const auto *A = cast_or_null<WarnUnusedResultAttr>(
262-
CE->getUnusedResultAttr(Context))) {
263-
StringRef Msg = A->getMessage();
264-
if (!Msg.empty())
265-
Diag(Loc, diag::warn_unused_result_msg) << A << Msg << R1 << R2;
266-
else
267-
Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
285+
if (DiagnoseNoDiscard(*this, cast_or_null<WarnUnusedResultAttr>(
286+
CE->getUnusedResultAttr(Context)),
287+
Loc, R1, R2, /*isCtor=*/false))
268288
return;
269-
}
270289

271290
// If the callee has attribute pure, const, or warn_unused_result, warn with
272291
// a more specific message to make it clear what is happening. If the call
@@ -284,24 +303,34 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
284303
return;
285304
}
286305
}
306+
} else if (const auto *CE = dyn_cast<CXXConstructExpr>(E)) {
307+
if (const CXXConstructorDecl *Ctor = CE->getConstructor()) {
308+
const auto *A = Ctor->getAttr<WarnUnusedResultAttr>();
309+
A = A ? A : Ctor->getParent()->getAttr<WarnUnusedResultAttr>();
310+
if (DiagnoseNoDiscard(*this, A, Loc, R1, R2, /*isCtor=*/true))
311+
return;
312+
}
313+
} else if (const auto *ILE = dyn_cast<InitListExpr>(E)) {
314+
if (const TagDecl *TD = ILE->getType()->getAsTagDecl()) {
315+
316+
if (DiagnoseNoDiscard(*this, TD->getAttr<WarnUnusedResultAttr>(), Loc, R1,
317+
R2, /*isCtor=*/false))
318+
return;
319+
}
287320
} else if (ShouldSuppress)
288321
return;
289322

323+
E = WarnExpr;
290324
if (const ObjCMessageExpr *ME = dyn_cast<ObjCMessageExpr>(E)) {
291325
if (getLangOpts().ObjCAutoRefCount && ME->isDelegateInitCall()) {
292326
Diag(Loc, diag::err_arc_unused_init_message) << R1;
293327
return;
294328
}
295329
const ObjCMethodDecl *MD = ME->getMethodDecl();
296330
if (MD) {
297-
if (const auto *A = MD->getAttr<WarnUnusedResultAttr>()) {
298-
StringRef Msg = A->getMessage();
299-
if (!Msg.empty())
300-
Diag(Loc, diag::warn_unused_result_msg) << A << Msg << R1 << R2;
301-
else
302-
Diag(Loc, diag::warn_unused_result) << A << R1 << R2;
331+
if (DiagnoseNoDiscard(*this, MD->getAttr<WarnUnusedResultAttr>(), Loc, R1,
332+
R2, /*isCtor=*/false))
303333
return;
304-
}
305334
}
306335
} else if (const PseudoObjectExpr *POE = dyn_cast<PseudoObjectExpr>(E)) {
307336
const Expr *Source = POE->getSyntacticForm();

‎clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p2.cpp

+50
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,50 @@ void cxx2a_use() {
8080
conflicting_reason(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: special reason}}
8181
}
8282

83+
namespace p1771 {
84+
struct[[nodiscard("Don't throw me away!")]] ConvertTo{};
85+
struct S {
86+
[[nodiscard]] S();
87+
[[nodiscard("Don't let that S-Char go!")]] S(char);
88+
S(int);
89+
[[gnu::warn_unused_result]] S(double);
90+
operator ConvertTo();
91+
[[nodiscard]] operator int();
92+
[[nodiscard("Don't throw away as a double")]] operator double();
93+
};
94+
95+
struct[[nodiscard("Don't throw me away either!")]] Y{};
96+
97+
void usage() {
98+
S(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute}}
99+
S('A'); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
100+
S(1);
101+
S(2.2);
102+
Y(); // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away either!}}
103+
S s;
104+
ConvertTo{}; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
105+
106+
// AST is different in C++20 mode, pre-2017 a move ctor for ConvertTo is there
107+
// as well, hense the constructor warning.
108+
#if __cplusplus >= 201703L
109+
// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
110+
#else
111+
// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
112+
#endif
113+
(ConvertTo) s;
114+
(int)s; // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
115+
(S)'c'; // expected-warning {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't let that S-Char go!}}
116+
#if __cplusplus >= 201703L
117+
// expected-warning@+4 {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw me away!}}
118+
#else
119+
// expected-warning@+2 {{ignoring temporary created by a constructor declared with 'nodiscard' attribute: Don't throw me away!}}
120+
#endif
121+
static_cast<ConvertTo>(s);
122+
static_cast<int>(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
123+
static_cast<double>(s); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute: Don't throw away as a double}}
124+
}
125+
}; // namespace p1771
126+
83127
#ifdef EXT
84128
// expected-warning@5 {{use of the 'nodiscard' attribute is a C++17 extension}}
85129
// expected-warning@9 {{use of the 'nodiscard' attribute is a C++17 extension}}
@@ -91,4 +135,10 @@ void cxx2a_use() {
91135
// expected-warning@71 {{use of the 'nodiscard' attribute is a C++2a extension}}
92136
// expected-warning@73 {{use of the 'nodiscard' attribute is a C++2a extension}}
93137
// expected-warning@74 {{use of the 'nodiscard' attribute is a C++2a extension}}
138+
// expected-warning@84 {{use of the 'nodiscard' attribute is a C++2a extension}}
139+
// expected-warning@86 {{use of the 'nodiscard' attribute is a C++17 extension}}
140+
// expected-warning@87 {{use of the 'nodiscard' attribute is a C++2a extension}}
141+
// expected-warning@91 {{use of the 'nodiscard' attribute is a C++17 extension}}
142+
// expected-warning@92 {{use of the 'nodiscard' attribute is a C++2a extension}}
143+
// expected-warning@95 {{use of the 'nodiscard' attribute is a C++2a extension}}
94144
#endif

‎clang/test/Preprocessor/has_attribute.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ CXX11(unlikely)
6363
// CHECK: maybe_unused: 201603L
6464
// ITANIUM: no_unique_address: 201803L
6565
// WINDOWS: no_unique_address: 0
66-
// CHECK: nodiscard: 201603L
66+
// CHECK: nodiscard: 201907L
6767
// CHECK: noreturn: 200809L
6868
// FIXME(201803L) CHECK: unlikely: 0
6969

‎clang/www/cxx_status.html

+1-1
Original file line numberDiff line numberDiff line change
@@ -664,7 +664,7 @@ <h2 id="cxx17">C++17 implementation status</h2>
664664
</tr>
665665
<tr> <!-- from Cologne 2019 -->
666666
<td><a href="http://wg21.link/p1771r1">P1771R1</a> (<a href="#dr">DR</a>)</td>
667-
<td class="none" align="center">No</td>
667+
<td class="none" align="center">SVN</td>
668668
</tr>
669669
<tr>
670670
<td><tt>[[maybe_unused]]</tt> attribute</td>

0 commit comments

Comments
 (0)
Please sign in to comment.