Page MenuHomePhabricator

[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

There are a very large number of changes, so older changes are hidden. Show Older Changes
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.

125

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

133

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

178–179

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
178–179

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.

178–179

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
104

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.