[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Alexander Kornienko via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL327833: [clang-tidy] New check bugprone-unused-return-value 
(authored by alexfh, committed by ).
Herald added subscribers: llvm-commits, klimek.

Changed prior to commit:
  https://reviews.llvm.org/D41655?vs=138595=138901#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D41655

Files:
  clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-unused-return-value.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/test/clang-tidy/bugprone-unused-return-value-custom.cpp
  clang-tools-extra/trunk/test/clang-tidy/bugprone-unused-return-value.cpp

Index: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -43,6 +43,7 @@
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
 #include "UnusedRaiiCheck.h"
+#include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
@@ -119,6 +120,8 @@
 "bugprone-undelegated-constructor");
 CheckFactories.registerCheck(
 "bugprone-unused-raii");
+CheckFactories.registerCheck(
+"bugprone-unused-return-value");
 CheckFactories.registerCheck(
 "bugprone-use-after-move");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
@@ -35,6 +35,7 @@
   UndefinedMemoryManipulationCheck.cpp
   UndelegatedConstructorCheck.cpp
   UnusedRaiiCheck.cpp
+  UnusedReturnValueCheck.cpp
   UseAfterMoveCheck.cpp
   VirtualNearMissCheck.cpp
 
Index: clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/UnusedReturnValueCheck.cpp
@@ -0,0 +1,82 @@
+//===--- 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 ) {
+  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(
+   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)));
+ 

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 138595.
khuttun added a comment.

Rebased


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,166 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+ForwardIt remove(ForwardIt, ForwardIt, const T &);
+
+template 
+ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate);
+
+template 
+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 
+T *launder(T *);
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future );
+
+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();
+  // 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 

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D41655#1037552, @khuttun wrote:

> In https://reviews.llvm.org/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 https://reviews.llvm.org/D17043 handles.

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


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

In https://reviews.llvm.org/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 https://reviews.llvm.org/D17043 handles.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

Do you need help committing the patch?


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh accepted this revision.
alexfh added a comment.

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).


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

I think this check LGTM.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

Should I close this review?


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 134802.
khuttun added a comment.

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.


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,166 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+ForwardIt remove(ForwardIt, ForwardIt, const T &);
+
+template 
+ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate);
+
+template 
+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 
+T *launder(T *);
+
+} // namespace v1
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future );
+
+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();
+  // 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 

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

> 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.




Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:45-48
+"^::std::async$|"
+"^::std::launder$|"
+"^::std::remove$|"
+"^::std::remove_if$|"

alexfh wrote:
> alexfh wrote:
> > 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.)
> 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.
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.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/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?


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:45-48
+"^::std::async$|"
+"^::std::launder$|"
+"^::std::remove$|"
+"^::std::remove_if$|"

alexfh wrote:
> 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.)
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.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:36
+  Node.printQualifiedName(OS, Policy);
+  return llvm::Regex(RegExp).match(OS.str());
+}

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.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:45-48
+"^::std::async$|"
+"^::std::launder$|"
+"^::std::remove$|"
+"^::std::remove_if$|"

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.)



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:61
+  callExpr(callee(functionDecl(
+   matchesInlinedName(FuncRegex),
+   // Don't match void overloads of checked functions.

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.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.
Herald added a subscriber: hintonda.

Any more comments on this?


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

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?


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 130461.
khuttun added a comment.

- 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


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,311 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T &);
+
+template 
+ForwardIt remove(ForwardIt, ForwardIt, const T &);
+
+template 
+ForwardIt remove_if(ForwardIt, ForwardIt, UnaryPredicate);
+
+template 
+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 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future );
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+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();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP2;
+  UP2.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator_traits::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  std::allocator_traits::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:47
+"^::std::launder$|"
+"^::std::unique_ptr<.*>::release$|"
+"^::std::.*::allocate$|"

khuttun wrote:
> JonasToth wrote:
> > Is the following type a problem for you check?
> > 
> > `std::unique_ptr` should not be matchable with regex but 
> > I don't know if that would have an impact on the functionality.
> `std::unique_ptr` is also matched. I added a test case for 
> it.
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 :)


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:210
+  // test that the check is disabled inside GNU statement expressions
+  ({ std::async(increment, 42); });
+  auto StmtExprRetval = ({ std::async(increment, 42); });

khuttun wrote:
> aaron.ballman wrote:
> > 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).
> 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.
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.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added a comment.

In https://reviews.llvm.org/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.




Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:47
+"^::std::launder$|"
+"^::std::unique_ptr<.*>::release$|"
+"^::std::.*::allocate$|"

JonasToth wrote:
> Is the following type a problem for you check?
> 
> `std::unique_ptr` should not be matchable with regex but I 
> don't know if that would have an impact on the functionality.
`std::unique_ptr` is also matched. I added a test case for it.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:210
+  // test that the check is disabled inside GNU statement expressions
+  ({ std::async(increment, 42); });
+  auto StmtExprRetval = ({ std::async(increment, 42); });

aaron.ballman wrote:
> 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).
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.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:47
+"^::std::launder$|"
+"^::std::unique_ptr<.*>::release$|"
+"^::std::.*::allocate$|"

Is the following type a problem for you check?

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


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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?


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:210
+  // test that the check is disabled inside GNU statement expressions
+  ({ std::async(increment, 42); });
+  auto StmtExprRetval = ({ std::async(increment, 42); });

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).


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 129090.
khuttun added a comment.

The checker is now disabled inside GNU statement expressions


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,212 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T &);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future );
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+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();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator_traits::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  std::allocator_traits::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added inline comments.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);

aaron.ballman wrote:
> khuttun wrote:
> > aaron.ballman wrote:
> > > khuttun wrote:
> > > > aaron.ballman wrote:
> > > > > 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 v;
> > > > > extern void f(bool);
> > > > > 
> > > > > f(v.empty()); // Should not warn
> > > > > ```
> > > > See line 199 in this file.
> > > Ah, my eyes missed that, thank you!
> > > 
> > > Hmm, I *think* this test should also be okay, but I want to be sure:
> > > ```
> > > std::vector 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.
> > 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"?
> > 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.
This was a good catch. Code like this

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

is currently causing a warning.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);

khuttun wrote:
> aaron.ballman wrote:
> > khuttun wrote:
> > > aaron.ballman wrote:
> > > > 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 v;
> > > > extern void f(bool);
> > > > 
> > > > f(v.empty()); // Should not warn
> > > > ```
> > > See line 199 in this file.
> > Ah, my eyes missed that, thank you!
> > 
> > Hmm, I *think* this test should also be okay, but I want to be sure:
> > ```
> > std::vector 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.
> 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"?
> 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.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun added inline comments.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);

aaron.ballman wrote:
> khuttun wrote:
> > aaron.ballman wrote:
> > > 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 v;
> > > extern void f(bool);
> > > 
> > > f(v.empty()); // Should not warn
> > > ```
> > See line 199 in this file.
> Ah, my eyes missed that, thank you!
> 
> Hmm, I *think* this test should also be okay, but I want to be sure:
> ```
> std::vector 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.
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"?


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);

khuttun wrote:
> aaron.ballman wrote:
> > 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 v;
> > extern void f(bool);
> > 
> > f(v.empty()); // Should not warn
> > ```
> See line 199 in this file.
Ah, my eyes missed that, thank you!

Hmm, I *think* this test should also be okay, but I want to be sure:
```
std::vector 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.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 128879.
khuttun added a comment.

Fix review comments


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,208 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T&);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future );
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+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();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator_traits::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  std::allocator_traits::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::vector FV;
+  FV.empty();
+  // 

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun marked 2 inline comments as done.
khuttun added inline comments.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);

aaron.ballman wrote:
> 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 v;
> extern void f(bool);
> 
> f(v.empty()); // Should not warn
> ```
See line 199 in this file.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:69
+ "the value returned by %0 should normally be used")
+<< dyn_cast_or_null(Matched->getCalleeDecl())
+<< Matched->getSourceRange();

khuttun wrote:
> aaron.ballman wrote:
> > 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.
> I can remove the function name from the diagnostic. Out of curiosity, in 
> which situations could it be null?
Situations where the call doesn't refer back to a declaration at all. For 
instance, a lambda or block.



Comment at: docs/clang-tidy/checks/bugprone-unused-return-value.rst:22
+   - ``std::unique_ptr::release()``. Not using the return value can lead to
+ resource leaks, if the same pointer isn't stored anywhere else. Often
+ ignoring the ``release()`` return value indicates that the programmer

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



Comment at: docs/clang-tidy/checks/bugprone-unused-return-value.rst:31
+ ``std::filesystem::path::empty()`` and ``std::empty()``. Not using the
+ return value doesn't cause any issues on itself, but often it indicates
+ that the programmer confused the function with ``clear()``.

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



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:163
+
+void noWarning() {
+  auto AsyncRetval1 = std::async(increment, 42);

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 v;
extern void f(bool);

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


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 128844.
khuttun added a comment.

Fix review comments


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,208 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T&);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future );
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+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();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::allocator_traits::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  std::allocator_traits::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by this function should be used [bugprone-unused-return-value]
+
+  std::vector FV;
+  FV.empty();
+  // 

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun marked an inline comment as done.
khuttun added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:69
+ "the value returned by %0 should normally be used")
+<< dyn_cast_or_null(Matched->getCalleeDecl())
+<< Matched->getSourceRange();

aaron.ballman wrote:
> 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.
I can remove the function name from the diagnostic. Out of curiosity, in which 
situations could it be null?


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:68
+diag(Matched->getLocStart(),
+ "the value returned by %0 should normally be used")
+<< dyn_cast_or_null(Matched->getCalleeDecl())

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



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:69
+ "the value returned by %0 should normally be used")
+<< dyn_cast_or_null(Matched->getCalleeDecl())
+<< Matched->getSourceRange();

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.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun updated this revision to Diff 128544.
khuttun added a comment.

Fix review comments


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,208 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+template 
+bool empty(const T&);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future );
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'async' should normally be used [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'async' should normally be used [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'launder' should normally be used [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'release' should normally be used [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+
+  std::allocator_traits::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+  std::allocator_traits::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: the value returned by 'allocate' should normally be used 

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun marked 7 inline comments as done.
khuttun added a comment.

In https://reviews.llvm.org/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.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:62-63
+void UnusedReturnValueCheck::check(const MatchFinder::MatchResult ) {
+  if (const auto Matched = Result.Nodes.getNodeAs("match")) {
+diag(Matched->getLocStart(), "unused return value");
+  }

`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.



Comment at: test/clang-tidy/bugprone-unused-return-value.cpp:200
+  increment(1);
+}

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


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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




Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:29
+  llvm::raw_svector_ostream OS(InlinedName);
+  auto Policy = Node.getASTContext().getPrintingPolicy();
+  Policy.SuppressUnwrittenScope = true;

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



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:53
+  // Detect unused return values by finding CallExprs with CompoundStmt parent,
+  // ignoring any implicit nodes and parentheses in between
+  Finder->addMatcher(

Missing full stop.


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:13
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;

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



Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.h:14
+#include "../ClangTidy.h"
+
+namespace clang {

Please include string.



Comment at: docs/clang-tidy/checks/bugprone-unused-return-value.rst:17
+
+   - ``std::async``. Not using the return value makes the call synchronous.
+   - ``std::launder``. Not using the return value usually means that the

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


https://reviews.llvm.org/D41655



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D41655: [clang-tidy] New check bugprone-unused-return-value

[Kalle Huttunen via Phabricator via cfe-commits]

khuttun created this revision.
khuttun added reviewers: alexfh, aaron.ballman.
khuttun added a project: clang-tools-extra.
Herald added subscribers: xazax.hun, mgorny.

Detects function calls where the return value is unused.

Checked functions can be configured.


https://reviews.llvm.org/D41655

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/bugprone/UnusedReturnValueCheck.cpp
  clang-tidy/bugprone/UnusedReturnValueCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/bugprone-unused-return-value.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/bugprone-unused-return-value-custom.cpp
  test/clang-tidy/bugprone-unused-return-value.cpp

Index: test/clang-tidy/bugprone-unused-return-value.cpp
===
--- /dev/null
+++ test/clang-tidy/bugprone-unused-return-value.cpp
@@ -0,0 +1,200 @@
+// RUN: %check_clang_tidy %s bugprone-unused-return-value %t
+
+namespace std {
+
+using size_t = decltype(sizeof(int));
+
+using max_align_t = long double;
+
+struct future {};
+
+enum class launch {
+  async,
+  deferred
+};
+
+template 
+future async(Function &&, Args &&...);
+
+template 
+future async(launch, Function &&, Args &&...);
+
+// the check should be able to match std lib calls even if the functions are
+// declared inside inline namespaces
+inline namespace v1 {
+
+template 
+T *launder(T *);
+
+} // namespace v1
+
+template 
+struct allocator {
+  using value_type = T;
+  T *allocate(std::size_t);
+  T *allocate(std::size_t, const void *);
+};
+
+template 
+struct allocator_traits {
+  using value_type = typename Alloc::value_type;
+  using pointer = value_type *;
+  using size_type = size_t;
+  using const_void_pointer = const void *;
+  static pointer allocate(Alloc &, size_type);
+  static pointer allocate(Alloc &, size_type, const_void_pointer);
+};
+
+template 
+struct scoped_allocator_adaptor : public OuterAlloc {
+  using pointer = typename allocator_traits::pointer;
+  using size_type = typename allocator_traits::size_type;
+  using const_void_pointer = typename allocator_traits::const_void_pointer;
+  pointer allocate(size_type);
+  pointer allocate(size_type, const_void_pointer);
+};
+
+template 
+struct default_delete {};
+
+template >
+struct unique_ptr {
+  using pointer = T *;
+  pointer release() noexcept;
+};
+
+template >
+struct vector {
+  bool empty() const noexcept;
+};
+
+namespace filesystem {
+
+struct path {
+  bool empty() const noexcept;
+};
+
+} // namespace filesystem
+
+namespace pmr {
+
+struct memory_resource {
+  void *allocate(size_t, size_t = alignof(max_align_t));
+};
+
+template 
+struct polymorphic_allocator {
+  T *allocate(size_t);
+};
+
+} // namespace pmr
+
+} // namespace std
+
+struct Foo {
+  void f();
+};
+
+int increment(int i) {
+  return i + 1;
+}
+
+void useFuture(const std::future );
+
+struct FooAlloc {
+  using value_type = Foo;
+};
+
+void warning() {
+  std::async(increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::async(std::launch::async, increment, 42);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  Foo F;
+  std::launder();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::unique_ptr UP;
+  UP.release();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::allocator FA;
+  FA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+  FA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::allocator_traits::allocate(FA, 1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+  std::allocator_traits::allocate(FA, 1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::scoped_allocator_adaptor SAA;
+  SAA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+  SAA.allocate(1, nullptr);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::pmr::memory_resource MR;
+  MR.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::pmr::polymorphic_allocator PA;
+  PA.allocate(1);
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::vector FV;
+  FV.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+
+  std::filesystem::path P;
+  P.empty();
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: unused return value [bugprone-unused-return-value]
+}
+