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:
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
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
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.
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
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
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
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
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.
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
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
alexfh added inline comments.
Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:45-48
+"^::std::async$|"
+"^::std::launder$|"
+"^::std::remove$|"
+
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
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
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
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
JonasToth added inline comments.
Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:47
+"^::std::launder$|"
+"^::std::unique_ptr<.*>::release$|"
+"^::std::.*::allocate$|"
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 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
JonasToth added inline comments.
Comment at: clang-tidy/bugprone/UnusedReturnValueCheck.cpp:47
+"^::std::launder$|"
+"^::std::unique_ptr<.*>::release$|"
+"^::std::.*::allocate$|"
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
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 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
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 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:
> >
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
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
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
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
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
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
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())
+<<
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"
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
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?
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
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
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.
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
39 matches
Mail list logo