[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