[clang-tidy] New check bugprone-unused-return-value

[khuttun]

Summary

Detects function calls where the return value is unused.

Checked functions can be configured.

[Eugene.Zelenko]

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

13 ↗

(On Diff #128386)

Please include cassert, Regex.h, raw_ostream.h, SmallString.h.

clang-tidy/bugprone/UnusedReturnValueCheck.h

14 ↗

(On Diff #128386)

Please include string.

docs/clang-tidy/checks/bugprone-unused-return-value.rst

17 ↗

(On Diff #128386)

Please add round brackets to all functions/methods here and below.

[JonasToth]

I think it would be more user friendly if the configured list can be a list and the | concatenation is done within your code.

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

29 ↗

(On Diff #128386)

Don't use auto here, because the type is not written somewhere

53 ↗

(On Diff #128386)

Missing full stop.

[aaron.ballman]

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

62–63 ↗

(On Diff #128386)

const auto Matched -> const auto *Matched

You can elide the curly braces.

The diagnostic doesn't really help the user understand what's wrong with the code. How about "the value returned by this function should be used" and add a note diagnostic that says "cast expression to void to silence warning". Also, you should pass in the SourceRange for the function call expression to the call to diag() so that it highlights the call in question.

test/clang-tidy/bugprone-unused-return-value.cpp

200 ↗

(On Diff #128386)

Can you also add a test for ignoring the result of a call to std::empty(SomeContainerWithAnEmptyMethod)?

[khuttun]

In D41655#965551, @JonasToth wrote:

I think it would be more user friendly if the configured list can be a list and the | concatenation is done within your code.

What do you exactly mean by list? It seems some other checks use comma or semicolon separators in their configuration strings, but is that really more user friendly than using "|"? At least now it's clear that the whole string is a regex.

[khuttun]

Fix review comments

[aaron.ballman]

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

68 ↗

(On Diff #128544)

"Normally" is probably a bad term to use here. How about "the value returned by %0 is usually not intended to be discarded"?

69 ↗

(On Diff #128544)

In the event this returns null, the diagnostic is going to look rather odd. Because the value of the call expression is unused, this will most often trigger in a context where the method call can be inferred (especially because you're now highlighting the source range). It might make sense to simply replace the %0 with "this call expression" or somesuch in the diagnostic.

[khuttun]

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

69 ↗

(On Diff #128544)

I can remove the function name from the diagnostic. Out of curiosity, in which situations could it be null?

[khuttun]

Fix review comments

[aaron.ballman]

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

69 ↗

(On Diff #128544)

Situations where the call doesn't refer back to a declaration at all. For instance, a lambda or block.

docs/clang-tidy/checks/bugprone-unused-return-value.rst

22 ↗

(On Diff #128844)

resource leaks, if the -> resource leaks if the
Often -> Often,

31 ↗

(On Diff #128844)

I'd reword this sentence to: Not using the return value often indicates that the programmer confused the function with clear().

test/clang-tidy/bugprone-unused-return-value.cpp

163 ↗

(On Diff #128844)

Sorry, I just realized that we're missing a test case for a common situation -- where the result is used as part of another call expression. Can you add a test to noWarning() to make sure this does not warn:

std::vector<int> v;
extern void f(bool);

f(v.empty()); // Should not warn

[khuttun]

test/clang-tidy/bugprone-unused-return-value.cpp

163 ↗

(On Diff #128844)

See line 199 in this file.

[khuttun]

Fix review comments

[aaron.ballman]

test/clang-tidy/bugprone-unused-return-value.cpp

163 ↗

(On Diff #128844)

Ah, my eyes missed that, thank you!

Hmm, I *think* this test should also be okay, but I want to be sure:

std::vector<int> v;
bool b = ({v.empty()}); // Should not warn
({v.empty()}); // ???

I kind of thing that, as an extension to the spirit of this check, any GNU expression statement whose result is unused should probably be diagnosed; what do you think?

You should add tests for the above so we document the expected behavior here.

[khuttun]

test/clang-tidy/bugprone-unused-return-value.cpp

163 ↗

(On Diff #128844)

Getting a warning when just surrounding the call expression with parenthesis is tested in bugprone-unused-return-value-custom.cpp.

Would your example be parsed as creating an initializer_list? In that case it should not warn. I can add test cases for that.

What do you mean by "GNU expression"?

[aaron.ballman]

test/clang-tidy/bugprone-unused-return-value.cpp

163 ↗

(On Diff #128844)

Getting a warning when just surrounding the call expression with parenthesis is tested in bugprone-unused-return-value-custom.cpp.

Yup, this is something entirely different, however.

Would your example be parsed as creating an initializer_list? In that case it should not warn. I can add test cases for that.

What do you mean by "GNU expression"?

Those examples are GNU expression statements, which is a GCC extension that Clang also supports. See https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html for more information.

Effectively, the last statement in the GNU expression statement is the "return value" of the expression statement. We should make sure the warnings are sensible with this construct.

[khuttun]

test/clang-tidy/bugprone-unused-return-value.cpp

163 ↗

(On Diff #128844)

This was a good catch. Code like this

auto Gnu = ({ fun(); });

is currently causing a warning.

[khuttun]

The checker is now disabled inside GNU statement expressions

[aaron.ballman]

test/clang-tidy/bugprone-unused-return-value.cpp

210 ↗

(On Diff #129090)

Not diagnosing this case is questionable -- the return value *is* unused and that's a bad thing. However, this is likely to be an uncommon code pattern, so I'd be fine if you added a FIXME to the AST matcher code that excludes GNU expression statements to handle this case someday, and add a FIXME comment here as well so we know what we would like the behavior to be (you could fix the FIXMEs in a follow-up patch if you'd like).

[JonasToth]

High Integrity C++ has the rule 17.5.1 Do not ignore the result of std::remove, std::remove_if or std::unique. Do we want to add those to the preconfigured list?

[JonasToth]

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

47 ↗

(On Diff #129090)

Is the following type a problem for you check?

std::unique_ptr<std::vector<int>> should not be matchable with regex but I don't know if that would have an impact on the functionality.

[khuttun]

In D41655#974725, @JonasToth wrote:

High Integrity C++ has the rule 17.5.1 Do not ignore the result of std::remove, std::remove_if or std::unique. Do we want to add those to the preconfigured list?

To me these sound like reasonable additions. I can add them and run the checker against LLVM source to see if we get any false positives with them.

I also noticed that the checker currently misses unused values inside other kinds of statements than compound statement (if statements, case statements etc.). I will also update the checker to handle these.

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

47 ↗

(On Diff #129090)

std::unique_ptr<std::vector<int>> is also matched. I added a test case for it.

test/clang-tidy/bugprone-unused-return-value.cpp

210 ↗

(On Diff #129090)

I'm not so sure whether this code should cause a warning. I see it as equivalent to this

[]{ return std::async(increment, 42); }();

, where the return value of std::async is used in the return statement.

One situation related to the GNU statement expressions that the checker currently misses is if the return value is unused inside the statement expression at some other than the last statement. I can see if this could be somehow covered.

[aaron.ballman]

test/clang-tidy/bugprone-unused-return-value.cpp

210 ↗

(On Diff #129090)

It all depends on how far down the rabbit hole you want the check to go, I guess.

auto fn = []{ return std::async(increment, 42); };
fn(); // Could also be reasonable to diagnose

I'm fine leaving it alone, but I'd like to see test coverage and comments showing the cases were explicitly considered.

[JonasToth]

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

47 ↗

(On Diff #129090)

Alright. I think i had a error in my thinking. Because there are followup letters in the regex there are no problems. I thought it would end on the first >, but thats no the case! Thank you for clarification :)

[khuttun]

• Detect unused return values also inside other kinds of statements than compound statements
• Ignore void functions in the checker
• Check std::remove, std::remove_if and std::unique by default

[khuttun]

The checker reports 7 warnings on LLVM + Clang code bases, all on std::unique_ptr::release:

lib/Bitcode/Reader/BitReader.cpp:114:3

• release() called on moved-from unique_ptr
• no harm, just unnecessary

lib/ExecutionEngine/ExecutionEngine.cpp:149:7

• release() called before erasing unique_ptr from a container
• false positive?

lib/Target/AMDGPU/GCNIterativeScheduler.cpp:196:5

• release() called before assigning new pointer to unique_ptr
• false positive?

lib/AsmParser/LLParser.cpp:855:3

• get() + release() could be replaced with release()

tools/clang/lib/Lex/ModuleMap.cpp:791:3

• false positive?

tools/clang/tools/extra/clangd/Compiler.cpp:61:3

• get() + release() could potentially be replaced with release()?

unittests/Support/Casting.cpp:144:3

• release() called to avoid calling delete on a pointer to global object
• false positive?

[khuttun]

Any more comments on this?

[alexfh]

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

36 ↗

(On Diff #130461)

Can we avoid creating the regex on each match? For example, by changing the parameter type to llvm::Regex. If we need the matcher at all - see the comment below.

45–48 ↗

(On Diff #130461)

I strongly suspect that we could get away without regexs here. hasAnyName supports inline namespaces, so at least the first five entries here are covered. The main problem with std::unique_ptr<.*>::release is the need to match any template parameters. I *think*, we could adjust hasName to allow omitting template parameters (so that std::unique_ptr::release would match specializations of unique_ptr as well). The empty and allocate would need some research. Can we just list all specific classes where we care about empty? (Alternatively, can we match empty unqualified and check that it's somewhere inside std, but I don't like that much, since it would add inconsistency in how this check is configured.)

61 ↗

(On Diff #130461)

This is a rather expensive matcher and it is going to be run on each callExpr, which is a lot. Let's at least swap it with the unless(returns(voidType())) below to reduce the number of times it's called.

[alexfh]

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

45–48 ↗

(On Diff #130461)

A clarification: we could go with your current version and optimize this part later. But the option may start being used by someone and then will change - that would be nice to avoid.

Alternatively, we could switch to hasAnyName right away and leave only the functions that can be easily supported, and then figure out what to do with release, allocate and empty.

I'd probably prefer the latter.

[aaron.ballman]

In D41655#980672, @khuttun wrote:

The checker reports 7 warnings on LLVM + Clang code bases, all on std::unique_ptr::release:

lib/Bitcode/Reader/BitReader.cpp:114:3

• release() called on moved-from unique_ptr
• no harm, just unnecessary

Agreed.

lib/ExecutionEngine/ExecutionEngine.cpp:149:7

• release() called before erasing unique_ptr from a container
• false positive?

False positive.

lib/Target/AMDGPU/GCNIterativeScheduler.cpp:196:5

• release() called before assigning new pointer to unique_ptr
• false positive?

False positive, but a bad code smell given that the assignment operator will perform the release.

lib/AsmParser/LLParser.cpp:855:3

• get() + release() could be replaced with release()

tools/clang/lib/Lex/ModuleMap.cpp:791:3

• false positive?

False positive.

tools/clang/tools/extra/clangd/Compiler.cpp:61:3

• get() + release() could potentially be replaced with release()?

False positive.

unittests/Support/Casting.cpp:144:3

• release() called to avoid calling delete on a pointer to global object
• false positive?

False positive.

From what I can tell of these reports, they almost all boil down to ignoring the call to release() because the pointer is no longer owned by the unique_ptr. This is a pretty reasonable code pattern, but it also seems reasonable to expect users to cast the result to void to silence the warning, so I think this is fine. Have you checked any other large C++ code bases, like Qt or boost?

[khuttun]

From what I can tell of these reports, they almost all boil down to ignoring the call to release() because the pointer is no longer owned by the unique_ptr. This is a pretty reasonable code pattern, but it also seems reasonable to expect users to cast the result to void to silence the warning, so I think this is fine. Have you checked any other large C++ code bases, like Qt or boost?

Yep, there's already code also in LLVM where the release() return value is cast to void, for example in lib/Bitcode/Reader/BitReader.cpp. I haven't run the checker on other large code bases yet, but I can test also that.

clang-tidy/bugprone/UnusedReturnValueCheck.cpp

45–48 ↗

(On Diff #130461)

If the ultimate goal would be to have this checker working without regexes, then I agree that we shouldn't introduce a version that uses them in the config string, as changing that later would break things.

About creating a version of hasName that ignores template arguments: as I understand it we'd need a new PrintingPolicy to suppress printing the template argument list in NamedDecl::printQualifiedName. Is this correct?

WG21 P0600R1 lists 8 allocate() and 24 empty() functions in std that should be marked with [[nodiscard]]. We could list all of them separately in the checker config, but the config string would get quite long. Regex matching handles these nicely, but if the performance is unacceptable, we have to look for other ways or just skip checking these.

[khuttun]

I changed the checker to use hasAnyName. Checking for unique_ptr::release() and all the empty() functions is removed.

The checker doesn't report any warnings from LLVM + clang codebases now.

[khuttun]

Should I close this review?

[aaron.ballman]

I think this check LGTM.

[alexfh]

LG. Thank you!

BTW, there's an old related patch https://reviews.llvm.org/D17043, which mostly focuses on member functions of STL containers. It might be useful as a reference (there's a pretty extensive list of member functions and containers).

[alexfh]

Do you need help committing the patch?

[khuttun]

In D41655#1037234, @alexfh wrote:

Do you need help committing the patch?

Yes please, I don't have commit access to the repo.

I think the next step for improving this checker could be to make it work with class template member functions. That would allow to add checking also to those container functions that D17043 handles.

[alexfh]

In D41655#1037552, @khuttun wrote:

In D41655#1037234, @alexfh wrote:

Do you need help committing the patch?

Yes please, I don't have commit access to the repo.

The patch doesn't apply cleanly. Please rebase against current HEAD.

I think the next step for improving this checker could be to make it work with class template member functions. That would allow to add checking also to those container functions that D17043 handles.

Yep, sounds reasonable. Thank you for working on this!

[khuttun]

Rebased

clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "SuspiciousMissingCommaCheck.h"
 #include "SuspiciousSemicolonCheck.h"
 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
 #include "UnusedRaiiCheck.h"
+#include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 class BugproneModule : public ClangTidyModule {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<ThrowKeywordMissingCheck>(
         "bugprone-throw-keyword-missing");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "bugprone-undelegated-constructor");
     CheckFactories.registerCheck<UnusedRaiiCheck>(
         "bugprone-unused-raii");
+    CheckFactories.registerCheck<UnusedReturnValueCheck>(
+        "bugprone-unused-return-value");
     CheckFactories.registerCheck<UseAfterMoveCheck>(
         "bugprone-use-after-move");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
         "bugprone-virtual-near-miss");
   }
 };
 
 } // namespace bugprone

clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt

add_clang_library(clangTidyBugproneModule
   SuspiciousMissingCommaCheck.cpp
   SuspiciousSemicolonCheck.cpp
   SuspiciousStringCompareCheck.cpp
   SwappedArgumentsCheck.cpp
   ThrowKeywordMissingCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
   UnusedRaiiCheck.cpp
+  UnusedReturnValueCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.h

+//===--- UnusedReturnValueCheck.h - clang-tidy-------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNUSEDRETURNVALUECHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNUSEDRETURNVALUECHECK_H
+
+#include "../ClangTidy.h"
+#include <string>
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Detects function calls where the return value is unused.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unused-return-value.html
+class UnusedReturnValueCheck : public ClangTidyCheck {
+public:
+  UnusedReturnValueCheck(StringRef Name, ClangTidyContext *Context);
+  void storeOptions(ClangTidyOptions::OptionMap &Opts) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+  std::string CheckedFunctions;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNUSEDRETURNVALUECHECK_H

clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp

+//===--- UnusedReturnValueCheck.cpp - clang-tidy---------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "UnusedReturnValueCheck.h"
+#include "../utils/OptionsUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+using namespace clang::ast_matchers::internal;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+UnusedReturnValueCheck::UnusedReturnValueCheck(llvm::StringRef Name,
+                                               ClangTidyContext *Context)
+    : ClangTidyCheck(Name, Context),
+      CheckedFunctions(Options.get("CheckedFunctions", "::std::async;"
+                                                       "::std::launder;"
+                                                       "::std::remove;"
+                                                       "::std::remove_if;"
+                                                       "::std::unique")) {}
+
+void UnusedReturnValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
+  Options.store(Opts, "CheckedFunctions", CheckedFunctions);
+}
+
+void UnusedReturnValueCheck::registerMatchers(MatchFinder *Finder) {
+  auto FunVec = utils::options::parseStringList(CheckedFunctions);
+  auto MatchedCallExpr = expr(ignoringImplicit(ignoringParenImpCasts(
+      callExpr(
+          callee(functionDecl(
+              // Don't match void overloads of checked functions.
+              unless(returns(voidType())), hasAnyName(std::vector<StringRef>(
+                                               FunVec.begin(), FunVec.end())))))
+          .bind("match"))));
+
+  auto UnusedInCompoundStmt =
+      compoundStmt(forEach(MatchedCallExpr),
+                   // The checker can't currently differentiate between the
+                   // return statement and other statements inside GNU statement
+                   // expressions, so disable the checker inside them to avoid
+                   // false positives.
+                   unless(hasParent(stmtExpr())));
+  auto UnusedInIfStmt =
+      ifStmt(eachOf(hasThen(MatchedCallExpr), hasElse(MatchedCallExpr)));
+  auto UnusedInWhileStmt = whileStmt(hasBody(MatchedCallExpr));
+  auto UnusedInDoStmt = doStmt(hasBody(MatchedCallExpr));
+  auto UnusedInForStmt =
+      forStmt(eachOf(hasLoopInit(MatchedCallExpr),
+                     hasIncrement(MatchedCallExpr), hasBody(MatchedCallExpr)));
+  auto UnusedInRangeForStmt = cxxForRangeStmt(hasBody(MatchedCallExpr));
+  auto UnusedInCaseStmt = switchCase(forEach(MatchedCallExpr));
+
+  Finder->addMatcher(
+      stmt(anyOf(UnusedInCompoundStmt, UnusedInIfStmt, UnusedInWhileStmt,
+                 UnusedInDoStmt, UnusedInForStmt, UnusedInRangeForStmt,
+                 UnusedInCaseStmt)),
+      this);
+}
+
+void UnusedReturnValueCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Matched = Result.Nodes.getNodeAs<CallExpr>("match")) {
+    diag(Matched->getLocStart(),
+         "the value returned by this function should be used")
+        << Matched->getSourceRange();
+    diag(Matched->getLocStart(),
+         "cast the expression to void to silence this warning",
+         DiagnosticIDs::Note);
+  }
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 - New module ``zircon`` for checks related to Fuchsia's Zircon kernel.
 
 - New `bugprone-throw-keyword-missing
   <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-throw-keyword-missing.html>`_ check
 
   Diagnoses when a temporary object that appears to be an exception is
   constructed but not thrown.
 
+- New `bugprone-unused-return-value
+  <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unused-return-value.html>`_ check
+
+  Warns on unused function return values.
+
 - New `cppcoreguidelines-avoid-goto
   <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-avoid-goto.html>`_ check
 
   The usage of ``goto`` for control flow is error prone and should be replaced
   with looping constructs. Every backward jump is rejected. Forward jumps are
   only allowed in nested loops.
 
 - New `fuchsia-multiple-inheritance

clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-unused-return-value.rst

+.. title:: clang-tidy - bugprone-unused-return-value
+
+bugprone-unused-return-value
+============================
+
+Warns on unused function return values. The checked funtions can be configured.
+
+Options
+-------
+
+.. option:: CheckedFunctions
+
+   Semicolon-separated list of functions to check. Defaults to
+   ``::std::async;::std::launder;::std::remove;::std::remove_if;::std::unique``.
+   This means that the calls to following functions are checked by default:
+
+   - ``std::async()``. Not using the return value makes the call synchronous.
+   - ``std::launder()``. Not using the return value usually means that the
+     function interface was misunderstood by the programmer. Only the returned
+     pointer is "laundered", not the argument.
+   - ``std::remove()``, ``std::remove_if()`` and ``std::unique()``. The returned
+     iterator indicates the boundary between elements to keep and elements to be
+     removed. Not using the return value means that the information about which
+     elements to remove is lost.

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    bugprone-suspicious-missing-comma
    bugprone-suspicious-semicolon
    bugprone-suspicious-string-compare
    bugprone-swapped-arguments
    bugprone-throw-keyword-missing
    bugprone-undefined-memory-manipulation
    bugprone-undelegated-constructor
    bugprone-unused-raii
+   bugprone-unused-return-value
    bugprone-use-after-move
    bugprone-virtual-near-miss
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl58-cpp
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>

clang-tools-extra/trunk/test/clang-tidy/bugprone-unused-return-value-custom.cpp

+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: bugprone-unused-return-value.CheckedFunctions, \
+// RUN:    value: "::fun;::ns::Outer::Inner::memFun;::ns::Type::staticFun"}]}' \
+// RUN: --
+
+namespace std {
+
+template <typename T>
+T *launder(T *);
+
+} // namespace std
+
+namespace ns {
+
+struct Outer {
+  struct Inner {
+    bool memFun();
+  };
+};
+
+using AliasName = Outer;
+
+struct Derived : public Outer::Inner {};
+
+struct Retval {
+  int *P;
+  Retval() { P = new int; }
+  ~Retval() { delete P; }
+};
+
+struct Type {
+  Retval memFun();
+  static Retval staticFun();
+};
+
+} // namespace ns
+
+int fun();
+void fun(int);
+
+void warning() {
+  fun();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  (fun());
+  // CHECK-MESSAGES: [[@LINE-1]]:4: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  ns::Outer::Inner ObjA1;
+  ObjA1.memFun();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  ns::AliasName::Inner ObjA2;
+  ObjA2.memFun();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  ns::Derived ObjA3;
+  ObjA3.memFun();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  ns::Type::staticFun();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+}
+
+void noWarning() {
+  auto R1 = fun();
+
+  ns::Outer::Inner ObjB1;
+  auto R2 = ObjB1.memFun();
+
+  auto R3 = ns::Type::staticFun();
+
+  // test calling a void overload of a checked function
+  fun(5);
+
+  // test discarding return value of functions that are not configured to be checked
+  int I = 1;
+  std::launder(&I);
+
+  ns::Type ObjB2;
+  ObjB2.memFun();
+}

clang-tools-extra/trunk/test/clang-tidy/bugprone-unused-return-value.cpp

+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template <typename Function, typename... Args>
+future async(Function &&, Args &&...);
+
+template <typename Function, typename... Args>
+future async(launch, Function &&, Args &&...);
+
+template <typename ForwardIt, typename T>
+ForwardIt remove(ForwardIt, ForwardIt, const T &);
+
+template <typename ForwardIt, typename UnaryPredicate>
+ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate);
+
+template <typename ForwardIt>
+ForwardIt unique(ForwardIt, ForwardIt);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template <typename T>
+T *launder(T *);
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future &fut);
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder(&F);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::remove_if(nullptr, nullptr, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique(nullptr, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  // test discarding return values inside different kinds of statements
+
+  auto Lambda = [] { std::remove(nullptr, nullptr, 1); };
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  if (true)
+    std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  else if (true)
+    std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  else
+    std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  while (true)
+    std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  do
+    std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  while (true);
+
+  for (;;)
+    std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  for (std::remove(nullptr, nullptr, 1);;)
+    // CHECK-MESSAGES: [[@LINE-1]]:8: warning: the value returned by this function should be used [bugprone-unused-return-value]
+    ;
+
+  for (;; std::remove(nullptr, nullptr, 1))
+    // CHECK-MESSAGES: [[@LINE-1]]:11: warning: the value returned by this function should be used [bugprone-unused-return-value]
+    ;
+
+  for (auto C : "foo")
+    std::remove(nullptr, nullptr, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  switch (1) {
+  case 1:
+    std::remove(nullptr, nullptr, 1);
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+    break;
+  default:
+    std::remove(nullptr, nullptr, 1);
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+    break;
+  }
+
+  try {
+    std::remove(nullptr, nullptr, 1);
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  } catch (...) {
+    std::remove(nullptr, nullptr, 1);
+    // CHECK-MESSAGES: [[@LINE-1]]:5: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  }
+}
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);
+  auto AsyncRetval2 = std::async(std::launch::async, increment, 42);
+
+  Foo FNoWarning;
+  auto LaunderRetval = std::launder(&FNoWarning);
+
+  auto RemoveRetval = std::remove(nullptr, nullptr, 1);
+
+  auto RemoveIfRetval = std::remove_if(nullptr, nullptr, nullptr);
+
+  auto UniqueRetval = std::unique(nullptr, nullptr);
+
+  // test using the return value in different kinds of expressions
+  useFuture(std::async(increment, 42));
+  std::launder(&FNoWarning)->f();
+  delete std::launder(&FNoWarning);
+
+  if (std::launder(&FNoWarning))
+    ;
+  for (; std::launder(&FNoWarning);)
+    ;
+  while (std::launder(&FNoWarning))
+    ;
+  do
+    ;
+  while (std::launder(&FNoWarning));
+  switch (std::unique(1, 1))
+    ;
+
+  // cast to void should allow ignoring the return value
+  (void)std::async(increment, 42);
+
+  // test discarding return value of functions that are not configured to be checked
+  increment(1);
+
+  // test that the check is disabled inside GNU statement expressions
+  ({ std::async(increment, 42); });
+  auto StmtExprRetval = ({ std::async(increment, 42); });
+}