Adds a clang-tidy check for the incorrect use of empty() on a
container when the result of the call is ignored.
Authored-by: Abraham Corea Diaz <abrahamcd@google.com>
Co-authored-by: Denis Nikitin <denik@google.com>
Paths
| Differential D128372
[Clang-Tidy] Empty Check ClosedPublic Authored by denik on Jun 22 2022, 12:33 PM.
Details Summary Adds a clang-tidy check for the incorrect use of empty() on a Authored-by: Abraham Corea Diaz <abrahamcd@google.com>
Diff Detail
Event TimelineComment Actions A great start, thanks!
Comment Actions
Ack. I still think this CL is useful, given that not every library will have [[nodiscard]], and because it can suggest appropriate alternatives.
Comment Actions Clang-Tidy tests and docs have moved to module subdirectories. Please rebase this onto main:HEAD and:
This revision now requires changes to proceed.Jun 23 2022, 10:39 AM abrahamcd marked 7 inline comments as done. Comment ActionsAdded functionality to check if member function clear() exists before
LegalizeAdulthood added inline comments.
This revision now requires changes to proceed.Jun 25 2022, 1:16 PM Comment Actions Removed old test file.
Comment Actions Thanks for adding a check! Please check my comments.
Comment Actions Progress Update Hit a roadblock with determining unused return value, Comment Actions Adds functionality for only warning on the case of unused return value
abrahamcd marked 3 inline comments as done. Comment ActionsAdds argument 0 test case and refactoring based on feedback. Comment Actions Thanks for working on this! I'll merge this on your behalf once there's maintainer approval. Comment Actions Hi all, I just wanted to check in on this again and see if any more feedback or progress could be made. Thank you! rsmith added inline comments.
abrahamcd marked 5 inline comments as done. Comment ActionsAdds check for the compatibility of qualifiers on the clear method and the object it is called on. abrahamcd marked 2 inline comments as done. Comment ActionsFormatting fixes and synchronization of documentation. Comment Actions This patch has been open for weeks without maintainer input, despite repeated requests, and @abrahamcd finishes his internship this Friday. We would very much appreciate direction on whether or not D128372 is ready to be merged. Comment Actions Drive-by comments: this patch may need a review from common reviewers like @njames93. I assume that @LegalizeAdulthood's comment (about the file hierarchy since there was a big reorganization few months ago) has been addressed. If you are concerned with unable to contact @LegalizeAdulthood, I think folks like @njames93 can decide moving forward the patch despite the "Request Changes" state. That said, it'll always be good to give @LegalizeAdulthood some time when the patch is in a ready-to-submit state. Comment Actions @LegalizeAdulthood, @njames93, is there anything else we should address in this change? Comment Actions @LegalizeAdulthood @njames93 is there anything actionable that's left for this patch? Comment Actions @Eugene.Zelenko , could you please stamp the patch if you don't have any other concerns? @njames93 , if you don't have spare cycles could you please suggest other reviewers? Comment Actions
Please wait for developers approval. I look only for trivial issues. Comment Actions @njames93 if you don't have any further concerns, I am going to merge this on Friday afternoon, as it will have been four months since there has been a maintainer's input. Comment Actions
@cjdb I have raised a concern about the lack of reviewer feedback in clang-tidy here: If more people express their concerns perhaps it can be escalated to the point where action is taken? Comment Actions I just took a glance and the code looks reasonable.
denik marked 2 inline comments as done. Comment ActionsFormat changes. Removed blank lines and fixed test types. Comment Actions
@MaskRay thanks for the review! I have updated the patch. Closed by commit rGec3f8feddf81: [Clang-Tidy] Empty Check (authored by abrahamcd, committed by cjdb). · Explain Why This revision was automatically updated to reflect the committed changes. Comment Actions Ironically as an aside... looking back at the review because of the discource article. The whole reason why I added the [[nodiscard]] checker to clang-tidy 4 years ago was to cover exactly this use case...and many others like it where users have a function which returns a boolean and takes no arguments are pointer or reference (empty() included) https://reviews.llvm.org/D55433 Shouldn't people be using this checker to add nodiscard and let the compiler to the heavy lifting. Was there a use case as to why that solution doesn't solve this? Comment Actions
That check is for library authors: this one is for library users. There are a non-insignificant number of cases where an unused call to empty pops up, hence the desire for this check.
Revision Contents
Diff 439131 clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp
clang-tools-extra/clang-tidy/bugprone/CMakeLists.txt
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.h
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/bugprone-standalone-empty.cpp
|
Please fix this :)