This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] readability-container-size-empty: simplify implementation
ClosedPublic

Authored by steveire on Nov 11 2020, 3:14 PM.

Details

Summary

Use IgnoreUnlessSpelledInSource to make the matcher code smaller and
more visibly-related to the code.

Diff Detail

Event Timeline

steveire created this revision.Nov 11 2020, 3:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 11 2020, 3:14 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
steveire requested review of this revision.Nov 11 2020, 3:14 PM
alexfh requested changes to this revision.Dec 14 2020, 5:29 PM

Pre-merge builder can't apply this patch: https://buildkite.com/llvm-project/diff-checks/builds/18651
Is it based on https://reviews.llvm.org/D91302 ? Do we need the intermediate state? Maybe squash the two patches together for simplicity?

This revision now requires changes to proceed.Dec 14 2020, 5:29 PM
aaron.ballman added inline comments.
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
56

It looks like the changes drop the requirement that either operand is 0 or 1 -- is that an oversight or can you explain why that's correct?

101

We seem to lose this case entirely?

steveire added inline comments.Dec 22 2020, 9:21 AM
clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
56

Sorry, that was an old/intermediate version of the patch.

101

In IgnoreUnlessSpelledInSource mode, ignoringImpCasts(cxxBindTemporaryExpr(has(DefaultConstructor))) simplifies to cxxConstructExpr(isDefaultConstruction()), which is there.

lebedev.ri retitled this revision from Simplify implementation of container-size-empty to [clang-tidy] readability-container-size-empt: simplify implementation.Dec 22 2020, 9:53 AM
lebedev.ri retitled this revision from [clang-tidy] readability-container-size-empt: simplify implementation to [clang-tidy] readability-container-size-empty: simplify implementation.Dec 22 2020, 9:53 AM
aaron.ballman accepted this revision.Dec 22 2020, 10:50 AM

LGTM!

clang-tools-extra/clang-tidy/readability/ContainerSizeEmptyCheck.cpp
101

Ah, exactly right, thanks!

This revision is now accepted and ready to land.Jan 19 2021, 5:58 PM
alexfh added inline comments.Jan 19 2021, 6:01 PM
clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
480–482 ↗(On Diff #313381)

Wait, is losing these diagnostics necessary?

aaron.ballman added inline comments.Jan 20 2021, 7:07 AM
clang-tools-extra/test/clang-tidy/checkers/readability-container-size-empty.cpp
480–482 ↗(On Diff #313381)

I think it's an improvement. The noexcept-ness of size() need not relate to the noexcept-ness of empty() (notionally it should, but it's not a requirement users must follow), so there's no guarantee that the change will have the same semantic effect on the declaration of func().

This revision was landed with ongoing or failed builds.Feb 5 2021, 6:04 AM
This revision was automatically updated to reflect the committed changes.