This is an archive of the discontinued LLVM Phabricator instance.

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

Diff Detail

Event Timeline

abrahamcd created this revision.Jun 22 2022, 12:33 PM
abrahamcd requested review of this revision.Jun 22 2022, 12:33 PM
Herald added a project: Restricted Project. · View Herald Transcript
cjdb added a subscriber: cjdb.Jun 22 2022, 1:43 PM

A great start, thanks!

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
52–58

Note to other reviewers: I suggested this because we couldn't work out a better way to identify if .clear() exists (Abraham can best expand on the issues regarding how a matcher wasn't working).

76–79

It looks as though this is unconditionally suggesting clear (even if it doesn't exist), and doesn't suggest it in the message.

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.h
19

Please fix this :)

clang-tools-extra/docs/ReleaseNotes.rst
128
  • s/container/range
  • This should probably mention it suggests clear() if it's a member function.
clang-tools-extra/docs/clang-tidy/checks/list.rst
363

I don't think the changes in this file were intetional?

clang-tools-extra/test/clang-tidy/checkers/bugprone-standalone-empty.cpp
5–9 ↗(On Diff #439131)

Please remove clear from vector, and add the following vector_with_clear and vector_with_void_empty and vector_with_int_empty.

We should be testing that clear() is only a suggested fix if it's present (but x.empty() is flagged either way), and only when x.empty() returns an integral result.

17–21 ↗(On Diff #439131)

As above. Please also rename absl::vector to something like absl::string (and drop the template altogether).

cjdb added a comment.Jun 22 2022, 2:03 PM

Ack. I still think this CL is useful, given that not every library will have [[nodiscard]], and because it can suggest appropriate alternatives.

cjdb added inline comments.Jun 22 2022, 2:04 PM
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
62

I wonder if we should add a fixit hint that suggests x = T{}; in the event x.clear() isn't available (e.g. std::stack).

Please add documentation for check.

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

Path to documentation was changed recently.

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

Please revert all changes made because of incorrect rebase.

Tests locations were changed recently too.

LegalizeAdulthood requested changes to this revision.Jun 23 2022, 10:39 AM

Clang-Tidy tests and docs have moved to module subdirectories.

Please rebase this onto main:HEAD and:

  • fold your changes into the appropriate subdirs, stripping the module prefix from new files.
  • make the target check-clang-extra to validate your tests
  • make the target docs-clang-tools-html to validate any docs changes
This revision now requires changes to proceed.Jun 23 2022, 10:39 AM
abrahamcd updated this revision to Diff 439938.Jun 24 2022, 5:26 PM
abrahamcd marked 7 inline comments as done.
abrahamcd retitled this revision from Clang-Tidy Empty Check to [Clang-Tidy] Empty Check.

Added functionality to check if member function clear() exists before
suggesting it as a fix, including updated tests to reflect that. Fixed
path change and rebase errors from the initial revision.

Eugene.Zelenko added inline comments.Jun 24 2022, 5:34 PM
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
56

Please don't use auto unless type is explicitly stated in same statement or iterator.

77

Please don't use auto unless type is explicitly stated in same statement or iterator.

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.h
23

Please fix URL.

30

Please define isLanguageVersionSupported().

clang-tools-extra/docs/clang-tidy/checks/bugprone/standalone-empty.rst
6

Excessive newline.

10

Boolean.

LegalizeAdulthood requested changes to this revision.Jun 25 2022, 1:16 PM
LegalizeAdulthood added inline comments.
clang-tools-extra/test/clang-tidy/checkers/bugprone-standalone-empty.cpp
1 ↗(On Diff #439938)

This file should be removed from the changelist

This revision now requires changes to proceed.Jun 25 2022, 1:16 PM
abrahamcd updated this revision to Diff 440286.Jun 27 2022, 9:54 AM

Removed old test file.

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
52–58

Yes, I wrote a matcher for the declaration of a .clear() member function and another matcher for a call to .empty(), but it seemed like the check() function runs multiple times to match the different matchers. It would find .clear() on one run of check() and find .empty() on another, and I couldn't find a way to see if .clear() had already been found on a previous run of check().

abrahamcd marked 7 inline comments as done.Jun 27 2022, 11:57 AM

Addressed review edits and clarity feedback.

Thanks for adding a check! Please check my comments.

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
77

Check getAsCXXRecordDecl() for null and add a test case.

clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
62

I think you also need no-warning test cases when the empty() result is used.

cjdb added inline comments.Jun 27 2022, 2:41 PM
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
77

We should also have a test to ensure empty(0) doesn't segfault.

abrahamcd updated this revision to Diff 440742.Jun 28 2022, 1:23 PM

Progress Update

Hit a roadblock with determining unused return value,
uploading current state for further work.

abrahamcd planned changes to this revision.Jun 28 2022, 1:25 PM

Stuck on determining unused return value, uploaded current state for @denik and @cjdb to take a look.

abrahamcd updated this revision to Diff 443017.Jul 7 2022, 11:55 AM
abrahamcd edited the summary of this revision. (Show Details)

Adds functionality for only warning on the case of unused return value
of the call to empty() and removes using-directive.

Eugene.Zelenko added inline comments.Jul 7 2022, 12:02 PM
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
147

nullptr.

abrahamcd marked 2 inline comments as done.Jul 7 2022, 12:02 PM
cjdb added inline comments.Jul 7 2022, 12:14 PM
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
129

Let's eliminate some of these else-clauses by using early exits.

abrahamcd updated this revision to Diff 443056.Jul 7 2022, 1:55 PM
abrahamcd marked 3 inline comments as done.

Adds argument 0 test case and refactoring based on feedback.

cjdb accepted this revision.Jul 7 2022, 2:07 PM

Thanks for working on this! I'll merge this on your behalf once there's maintainer approval.

denik accepted this revision.Jul 7 2022, 2:22 PM

Thanks Abraham!

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 a subscriber: rsmith.Aug 3 2022, 1:45 PM
rsmith added inline comments.
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
24
150

This only looks for direct members, not ones found in a base class. Eg, this will not find clear() in llvm::SmallVector<T, N>. If you have a Sema object here, you could do a real name lookup; otherwise, you can use CXXRecordDecl::lookupInBases to do the lookup. You should be able to find some examples by searching for existing calls to that function.

abrahamcd updated this revision to Diff 450407.Aug 5 2022, 2:26 PM

Checks for clear() in base classes as well.

abrahamcd marked 3 inline comments as done.Aug 5 2022, 2:29 PM
abrahamcd updated this revision to Diff 450430.Aug 5 2022, 3:18 PM

Adds non-dependent base class test case.

njames93 added inline comments.Aug 5 2022, 11:56 PM
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
58–73

These using declarations are annoying, the common convention in tidy checks is to just use a global using namespace ast_matchers at the top of the file.

121–122

We also need to check qualifiers. If there exists a clear method but its unavailable due to it not being const when our member is const. The fix would result in a compile error.

124

dyn_cast isn't needed here as we have already checked we are dealing with a CXXMethodDecl.

132

The 'did you mean' diagnostics in clang all use a semicolon ; instead of a comma between the message and suggestion.

177–178

Diagnostics can accept args as const NamedDecl *, so you can omit the call to getQualifiedNameAsString().
The use of dyn_cast here is suspicious. If the CalleeDecl isn't a NamedDecl, this would assert when you try and call getQualifiedNameAsString, but equally I can't see any case why the CalleeDecl wouldn't be a NamedDecl. @rsmith Can you think of any situation where this could happen?

abrahamcd added inline comments.Aug 8 2022, 3:00 PM
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
177–178

Seems that without getQualifiedNameAsString() I get longer less-readable versions of the name, e.g. 'empty<std::vector<int> &>' instead of just 'std::empty'. Do you think the extra information is helpful here?

cjdb added inline comments.Aug 8 2022, 3:15 PM
clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
58–73

Adding them just after the namespaces are opened would be good.

I've had issues with refactoring llvm-project files in the past that have global using-directives, so I'd prefer it if we stuck with the using-declarations please. If they're a part of the preamble, they become additional lines that will be scrolled over.

121–122

Good catch. We probably shouldn't be suggesting clear() on a const object anyway.

177–178

I'm partial to 'std::empty' because that's presumably what the user will type, and so it'll be easier for them to understand. 'empty<std::vector<int> &>' isn't useful because we almost always defer to type deduction.

abrahamcd updated this revision to Diff 451209.Aug 9 2022, 10:44 AM
abrahamcd marked 5 inline comments as done.

Adds check for the compatibility of qualifiers on the clear method and the object it is called on.

abrahamcd updated this revision to Diff 451212.Aug 9 2022, 10:51 AM

Minor formatting/naming fixes.

abrahamcd marked 2 inline comments as done.Aug 16 2022, 2:40 PM

Hi, checking in on this again, thanks!

cjdb added a comment.Aug 22 2022, 11:13 AM

@njames93, @LegalizeAdulthood, are there any further concerns regarding this CL?

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

Please synchronize with first statement in documentation.

clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
630

Excessive newline.

abrahamcd updated this revision to Diff 454635.Aug 22 2022, 3:33 PM
abrahamcd marked 2 inline comments as done.

Formatting fixes and synchronization of documentation.

cjdb added a comment.Aug 25 2022, 9:01 AM

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.

denik commandeered this revision.Aug 26 2022, 1:57 PM
denik edited reviewers, added: abrahamcd; removed: denik.
denik edited the summary of this revision. (Show Details)Aug 26 2022, 2:04 PM
MaskRay added a subscriber: MaskRay.EditedSep 7 2022, 12:36 PM

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.

Any updates from the reviewers?

denik added a comment.Sep 21 2022, 3:39 PM

@LegalizeAdulthood, @njames93, is there anything else we should address in this change?

cjdb added a comment.Oct 12 2022, 5:07 PM

@LegalizeAdulthood @njames93 is there anything actionable that's left for this patch?

@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?

@Eugene.Zelenko , could you please stamp the patch if you don't have any other concerns?

Please wait for developers approval. I look only for trivial issues.

cjdb added a comment.Nov 29 2022, 12:31 PM

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

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

@cjdb I have raised a concern about the lack of reviewer feedback in clang-tidy here:
https://discourse.llvm.org/t/rfc-improve-code-review-process-for-clang-tidy/66740

If more people express their concerns perhaps it can be escalated to the point where action is taken?

I just took a glance and the code looks reasonable.

clang-tools-extra/clang-tidy/bugprone/StandaloneEmptyCheck.cpp
132

delete blank line

161

delete blank line

clang-tools-extra/test/clang-tidy/checkers/bugprone/standalone-empty.cpp
162

unneeded blank line

ditto below

They just make tests less compact without good values.

178

[[@LINE-1]] is deprecated lit syntax. Use [[#@LINE-1]]. Don't use the prevailing style in clang-tidy tests.

denik updated this revision to Diff 479677.Dec 2 2022, 10:46 AM
denik marked 2 inline comments as done.

Format changes.

Removed blank lines and fixed test types.

denik marked an inline comment as done.Dec 2 2022, 10:49 AM
denik updated this revision to Diff 479684.Dec 2 2022, 10:58 AM

Replaced deprecated [[@LINE-1]]

denik marked an inline comment as done.Dec 2 2022, 10:58 AM
denik added a comment.Dec 2 2022, 12:10 PM

I just took a glance and the code looks reasonable.

@MaskRay thanks for the review! I have updated the patch.

This revision was not accepted when it landed; it landed in state Needs Review.Dec 9 2022, 3:21 PM
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.

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?

cjdb added a comment.Dec 15 2022, 6:04 AM

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?

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.