After a bit of internal testing and discussion, I've updated the patch to
not look through pointers and references when checking if the return type
of a function has the warn_unused_result. This fixes a lot of bogus
warnings from functions like std::copy which return an iterator but for
which the return value doesn't matter, when the iterator is defined as a
raw pointer. The core idea behind the change is that a pointer or reference
return type don't imply a transfer of ownership of the underlying object in
a way that suggests the contents of the object should be accessed before
the object goes out of scope.
On Thu, Apr 2, 2015 at 7:20 AM, Aaron Ballman <[email protected]>
wrote:
> LGTM!
>
> ~Aaron
>
> On Wed, Apr 1, 2015 at 7:30 PM, Kaelyn Takata <[email protected]> wrote:
> > The previous implementation would copy the attribute from the class to
> > functions that have the class as their return type when the functions
> > are first declared. This proved to have two flaws:
> > 1) if the class is forward-declared without the attribute and a
> > function or method with the class as a its return type is declared,
> > and afterward the class is defined with warn_unused_result, the
> > function or method would never inherit the attribute, and
> > 2) the check simply failed for functions and methods that are part of
> > a template instantiation, regardless of whether the class with
> > warn_unused_result is part of a specific instantiation or part of
> > the template itself (presumably because those function/method
> > declaration does not hit the same code path as a non-template one
> > and so never inherits the attribute).
> >
> > The new approach is to instead modify the two places where a function or
> > method call is checked for the warn_unused_result attribute on the decl
> > by extending the checks to also look for the attribute on the decl's
> > return type.
> > </commit-message>
> >
> > I've unified the two locations that performed the check into a helper
> method
> > of FunctionDecl that returns true if the decl or its return type have the
> > warn_unused_result attribute, except if only the return type has the
> > attribute and the decl is a member of the return type (which avoids
> spurious
> > messages on e.g. assignment operators).
> >
> > Cheers,
> > Kaelyn
> >
> > _______________________________________________
> > cfe-commits mailing list
> > [email protected]
> > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> >
>
diff --git a/include/clang/AST/Decl.h b/include/clang/AST/Decl.h
index 3e3d79f..f8e1726 100644
--- a/include/clang/AST/Decl.h
+++ b/include/clang/AST/Decl.h
@@ -1965,6 +1965,13 @@ public:
return getType()->getAs<FunctionType>()->getCallResultType(getASTContext());
}
+ /// \brief Returns true if this function or its return type has the
+ /// warn_unused_result attribute. If the return type has the attribute and
+ /// this function is a method of the return type's class, then false will be
+ /// returned to avoid spurious warnings on member methods such as assignment
+ /// operators.
+ bool hasUnusedResultAttr() const;
+
/// \brief Returns the storage class as written in the source. For the
/// computed linkage of symbol, see getLinkage.
StorageClass getStorageClass() const { return StorageClass(SClass); }
diff --git a/lib/AST/Decl.cpp b/lib/AST/Decl.cpp
index 3efd24f..58d0b84 100644
--- a/lib/AST/Decl.cpp
+++ b/lib/AST/Decl.cpp
@@ -2823,6 +2823,18 @@ SourceRange FunctionDecl::getReturnTypeSourceRange() const {
return RTRange;
}
+bool FunctionDecl::hasUnusedResultAttr() const {
+ QualType RetType = getReturnType();
+ if (RetType->isRecordType()) {
+ const CXXRecordDecl *Ret = RetType->getAsCXXRecordDecl();
+ const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(this);
+ if (Ret && Ret->hasAttr<WarnUnusedResultAttr>() &&
+ !(MD && MD->getCorrespondingMethodInClass(Ret, true)))
+ return true;
+ }
+ return hasAttr<WarnUnusedResultAttr>();
+}
+
/// \brief For an inline function definition in C, or for a gnu_inline function
/// in C++, determine whether the definition will be externally visible.
///
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
index d13a11c..372d7e9 100644
--- a/lib/AST/Expr.cpp
+++ b/lib/AST/Expr.cpp
@@ -2161,12 +2161,16 @@ bool Expr::isUnusedResultAWarning(const Expr *&WarnE, SourceLocation &Loc,
// If this is a direct call, get the callee.
const CallExpr *CE = cast<CallExpr>(this);
if (const Decl *FD = CE->getCalleeDecl()) {
+ const FunctionDecl *Func = dyn_cast<FunctionDecl>(FD);
+ bool HasWarnUnusedResultAttr = Func ? Func->hasUnusedResultAttr()
+ : FD->hasAttr<WarnUnusedResultAttr>();
+
// If the callee has attribute pure, const, or warn_unused_result, warn
// about it. void foo() { strlen("bar"); } should warn.
//
// Note: If new cases are added here, DiagnoseUnusedExprResult should be
// updated to match for QoI.
- if (FD->hasAttr<WarnUnusedResultAttr>() ||
+ if (HasWarnUnusedResultAttr ||
FD->hasAttr<PureAttr>() || FD->hasAttr<ConstAttr>()) {
WarnE = this;
Loc = CE->getCallee()->getLocStart();
diff --git a/lib/Sema/SemaDecl.cpp b/lib/Sema/SemaDecl.cpp
index ce183d0..1bad38f 100644
--- a/lib/Sema/SemaDecl.cpp
+++ b/lib/Sema/SemaDecl.cpp
@@ -7489,23 +7489,10 @@ Sema::ActOnFunctionDeclarator(Scope *S, Declarator &D, DeclContext *DC,
// Handle attributes.
ProcessDeclAttributes(S, NewFD, D);
- QualType RetType = NewFD->getReturnType();
- const CXXRecordDecl *Ret = RetType->isRecordType() ?
- RetType->getAsCXXRecordDecl() : RetType->getPointeeCXXRecordDecl();
- if (!NewFD->isInvalidDecl() && !NewFD->hasAttr<WarnUnusedResultAttr>() &&
- Ret && Ret->hasAttr<WarnUnusedResultAttr>()) {
- const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(NewFD);
- // Attach WarnUnusedResult to functions returning types with that attribute.
- // Don't apply the attribute to that type's own non-static member functions
- // (to avoid warning on things like assignment operators)
- if (!MD || MD->getParent() != Ret)
- NewFD->addAttr(WarnUnusedResultAttr::CreateImplicit(Context));
- }
-
if (getLangOpts().OpenCL) {
// OpenCL v1.1 s6.5: Using an address space qualifier in a function return
// type declaration will generate a compilation error.
- unsigned AddressSpace = RetType.getAddressSpace();
+ unsigned AddressSpace = NewFD->getReturnType().getAddressSpace();
if (AddressSpace == LangAS::opencl_local ||
AddressSpace == LangAS::opencl_global ||
AddressSpace == LangAS::opencl_constant) {
diff --git a/lib/Sema/SemaStmt.cpp b/lib/Sema/SemaStmt.cpp
index 0ac9c89..b491f9f 100644
--- a/lib/Sema/SemaStmt.cpp
+++ b/lib/Sema/SemaStmt.cpp
@@ -236,7 +236,9 @@ void Sema::DiagnoseUnusedExprResult(const Stmt *S) {
// is written in a macro body, only warn if it has the warn_unused_result
// attribute.
if (const Decl *FD = CE->getCalleeDecl()) {
- if (FD->hasAttr<WarnUnusedResultAttr>()) {
+ const FunctionDecl *Func = dyn_cast<FunctionDecl>(FD);
+ if (Func ? Func->hasUnusedResultAttr()
+ : FD->hasAttr<WarnUnusedResultAttr>()) {
Diag(Loc, diag::warn_unused_result) << R1 << R2;
return;
}
diff --git a/test/SemaCXX/warn-unused-result.cpp b/test/SemaCXX/warn-unused-result.cpp
index 7bdb424..01bc457 100644
--- a/test/SemaCXX/warn-unused-result.cpp
+++ b/test/SemaCXX/warn-unused-result.cpp
@@ -44,6 +44,12 @@ void bah() {
}
namespace warn_unused_CXX11 {
+class Status;
+class Foo {
+ public:
+ Status doStuff();
+};
+
struct [[clang::warn_unused_result]] Status {
bool ok() const;
Status& operator=(const Status& x);
@@ -73,10 +79,23 @@ void lazy() {
(void)DoYetAnotherThing();
DoSomething(); // expected-warning {{ignoring return value}}
- DoSomethingElse(); // expected-warning {{ignoring return value}}
- DoAnotherThing(); // expected-warning {{ignoring return value}}
+ DoSomethingElse();
+ DoAnotherThing();
DoYetAnotherThing();
}
+
+template <typename T>
+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}}
+
+ auto func = []() { return Status(); };
+ func(); // expected-warning {{ignoring return value}}
+}
}
namespace PR17587 {
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
