This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Don't treat invalid branches as identical
ClosedPublic

Authored by ishaangandhi on Jun 22 2022, 5:16 PM.

Details

Summary

The clang-tidy check bugprone-branch-clone has a false positive if some symbols are undefined. This patch silences the warning when the two sides of a branch are invalid.

See //github.com/llvm/llvm-project/issues/56057 for the original issue.

Diff Detail

Event Timeline

ishaangandhi created this revision.Jun 22 2022, 5:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 5:16 PM
ishaangandhi requested review of this revision.Jun 22 2022, 5:16 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 22 2022, 5:16 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Move test to new file with -fix-errors flag

Check for error messages in test case

Include error: in error line

Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
26

auto could be used because type is spelled in same statement.

27

Ditto.

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

Documentation path was changed recently. Please also keep alphabetical order inside section.

njames93 added inline comments.Jun 22 2022, 10:36 PM
clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
26

No need for dyn_cast here, we already know it's an Expr so cast would be sufficient.

28

Surely we can bail if only one of the sides contains an errors.

ishaangandhi added inline comments.Jun 23 2022, 6:03 AM
clang-tools-extra/clang-tidy/bugprone/BranchCloneCheck.cpp
28

Makes no difference, since when only one of the side has errors, the two profile hashes will be different.

clang-tools-extra/docs/ReleaseNotes.rst
236
ishaangandhi added inline comments.Jun 23 2022, 6:05 AM
clang-tools-extra/docs/ReleaseNotes.rst
236

Also, it doesn't look alphabetized to me? Two points up is "performance-unnecessary-..." then its "bugprone-use-after...".

Use auto instead of Expr and llvm::cast instead of llvm::dyn_cast.

ishaangandhi marked 3 inline comments as done.Jun 23 2022, 6:09 AM
Eugene.Zelenko added inline comments.Jun 23 2022, 6:39 AM
clang-tools-extra/docs/ReleaseNotes.rst
236

Sorry, my mistake.

236

It'll be good idea to fix order of other entries too.

ishaangandhi added inline comments.Jun 23 2022, 8:58 AM
clang-tools-extra/docs/ReleaseNotes.rst
236

Happy to! Would you like that in this diff or a new one?

Eugene.Zelenko added inline comments.Jun 23 2022, 9:08 AM
clang-tools-extra/docs/ReleaseNotes.rst
236

This is trivial change and could be made in same commit.

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

This link is wrong, needs to use the bugprone/ directory.

Make the target docs-clang-tools-html to verify that all your links are correct.

LegalizeAdulthood requested changes to this revision.Jun 23 2022, 10:50 AM
This revision now requires changes to proceed.Jun 23 2022, 10:50 AM

Alphabetize release notes

ishaangandhi marked an inline comment as done.Jun 23 2022, 11:54 AM
ishaangandhi added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
236

Done

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

Sorry for being unclear, but I meant alphabetical order for check names, not beginning of sentences.

ishaangandhi marked an inline comment as done.

Change doc path

ishaangandhi marked 2 inline comments as done.Jun 23 2022, 1:14 PM
ishaangandhi added inline comments.Jun 23 2022, 1:20 PM
clang-tools-extra/docs/ReleaseNotes.rst
236

This is then unclear to me. How do you alphabetize by check name?

Eugene.Zelenko added inline comments.Jun 23 2022, 2:05 PM
clang-tools-extra/docs/ReleaseNotes.rst
236

altera-struct-pack-align, bugprone-infinite-loop, bugprone-use-after-move, etc.

ishaangandhi marked 3 inline comments as done.Jun 23 2022, 7:20 PM

Alphabetize some more

clang-tools-extra/docs/ReleaseNotes.rst
208–211

I'm confused. The link text says performance-unnecessary-value-param, but the link is to readability-suspicious-call-argument and neither appears to be an alias for the other.

It appears this mistake was already there and you just alphabetized the list. I'm going to fix it in main and alphabetize the list so you can rebase and just add your changes in isolation.

OK, I've pushed the link fix and the sorting fix to main so if you rebase again, it should just show your changes to the release notes.

LegalizeAdulthood requested changes to this revision.Jun 25 2022, 1:13 PM

Rebase against main to get updated docs

This revision now requires changes to proceed.Jun 25 2022, 1:13 PM
This revision is now accepted and ready to land.Jun 27 2022, 9:35 AM

@LegalizeAdulthood Thanks! I don't have commit rights to the repository, can you commit it on my behalf?

Does anybody on this thread have land permissions? If not, would anyone know who to tag?

njames93 added inline comments.Jun 28 2022, 4:56 PM
clang-tools-extra/test/clang-tidy/checkers/bugprone/branch-clone-unknown-expr.cpp
2

is -fix-errors necessary here, given we aren't verifying any fixits?

LegalizeAdulthood requested changes to this revision.Jun 29 2022, 8:17 AM

I can submit after you address additional comments by Nathan James.

This revision now requires changes to proceed.Jun 29 2022, 8:17 AM

Remove -fix-errors

ishaangandhi marked an inline comment as done.Jun 29 2022, 3:57 PM

I remember now, @njames93 :

Without the -fix-errors, the test fails as follows:

Command Output (stdout):

--

Running ['clang-tidy', '/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp', '-fix', '--checks=-*,bugprone-branch-clone', '-config={}', '--', '-std=c++11', '-nostdinc++']...

clang-tidy /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp -fix --checks=-*,bugprone-branch-clone -config={} -- -std=c++11 -nostdinc++ failed:

3 errors generated.

Error while processing /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp.

/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp:4:7: error: use of undeclared identifier 'unknown_expression_1' [clang-diagnostic-error]

  if (unknown_expression_1) {        //

      ^

/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp:5:15: error: use of undeclared identifier 'unknown_expression_2' [clang-diagnostic-error]

    function1(unknown_expression_2); //

              ^

/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/test/clang-tidy/checkers/bugprone/Output/branch-clone-unknown-expr.cpp.tmp.cpp:7:15: error: use of undeclared identifier 'unknown_expression_3' [clang-diagnostic-error]

    function2(unknown_expression_3); //

              ^

Found compiler errors, but -fix-errors was not specified.

I won't update the diff, since I assume you will want to see the buildkite failure for yourself.

Once you see it, can you either confirm -fix-errors was correct originally, or instruct me on how to fix this test failure?

Once you see it, can you either confirm -fix-errors was correct originally, or instruct me on how to fix this test failure?

-expect-clang-tidy-error is the technically correct flag to use, but I'm easy either way.

I can submit after you address additional comments by Nathan James.

This revision is now accepted and ready to land.Jul 1 2022, 10:45 AM

Please provide the name and email address you wish to use on the commit and I will submit.

Sure!

Ishaan Gandhi
ishaangandhi AT gmail DOT com