This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Update llvm-prefer-isa-or-dyn-cast-in-conditionals with new syntax
Needs ReviewPublic

Authored by njames93 on Aug 6 2022, 2:30 AM.
Tokens
"Like" token, awarded by bzcheeseman.

Details

Summary

In D123901, the or_null, and_nonnull templates were deprecated intended to be replaced with if_present and and_present.
In light of this, The clang-tidy check that enforces correct use of isa and dyn_cast should also be updated with this syntax.

Diff Detail

Event Timeline

njames93 created this revision.Aug 6 2022, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 2:30 AM
njames93 requested review of this revision.Aug 6 2022, 2:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2022, 2:30 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Eugene.Zelenko added inline comments.Aug 6 2022, 7:41 AM
clang-tools-extra/docs/ReleaseNotes.rst
322

Please use double back-ticks.

njames93 updated this revision to Diff 450705.Aug 7 2022, 11:04 PM
njames93 marked an inline comment as done.

Fix documentation.

bzcheeseman accepted this revision.Aug 8 2022, 6:46 PM

This is great, thank you for doing this! I'm not a competent reviewer for the actual clang-tidy code changes but the +1 for the idea :)

This revision is now accepted and ready to land.Aug 8 2022, 6:46 PM
whisperity added inline comments.
clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
13–14

Will the tests pass properly once the fixes are applied, even though the replaced code refers to a symbol (isa_and_present) that is not declared in the TU?

This is great, thank you for doing this! I'm not a competent reviewer for the actual clang-tidy code changes but the +1 for the idea :)

The problem with the approval here is that a single approval will turn the patch into a fully approved state, which breaks the dashboards for people added to the patch (i.e., other reviewers will think the patch is already approved, and thus perhaps will not consider putting effort into reviewing it!).

However, I think you should try the Award Token option from the menu on the right! Somewhere the awarded tokens should show up on the patch, tallying up support!

This revision now requires review to proceed.Aug 9 2022, 6:30 AM

This is great, thank you for doing this! I'm not a competent reviewer for the actual clang-tidy code changes but the +1 for the idea :)

The problem with the approval here is that a single approval will turn the patch into a fully approved state, which breaks the dashboards for people added to the patch (i.e., other reviewers will think the patch is already approved, and thus perhaps will not consider putting effort into reviewing it!).

However, I think you should try the Award Token option from the menu on the right! Somewhere the awarded tokens should show up on the patch, tallying up support!

Ah I had no idea, thanks for pointing that out. I was looking at the Code Review document (https://llvm.org/docs/CodeReview.html), I'll put up a patch to add a short section on this.

njames93 added inline comments.Aug 9 2022, 9:29 AM
clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
13–14

The current tests would fail if ran once the fixes are applied as isa_and_nonnull doesn't exist in this TU. Most tidy checks (for better or worse) don't check for existence of an identifier that is expected to be there when making replacements.

LegalizeAdulthood resigned from this revision.Mar 29 2023, 8:27 AM
njames93 updated this revision to Diff 512267.Apr 10 2023, 2:49 PM

Added dynamic checking for existence of isa_and_present before suggesting it as a replacement

njames93 updated this revision to Diff 512269.Apr 10 2023, 2:58 PM

Fix wrong patch upload

PiotrZSL added inline comments.Apr 10 2023, 4:00 PM
clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
39

allOf is redudant

46

no need for allOf, just put those unless directly under callExpr

47

this also doesn't look to be needed..

47

maybe check argument count first argumentCountIs(1)
it could be cheaper than hasAnyName

57

Let me quote You: "Rather than calling traverse, the canoncial way to handle this is by overriding the getCheckTraversalKind virtual function to return TK_IgnoreUnlessSpelledInSource"
In this case just return TK_AsIs.

59

Change name of this any, on first look I mistook this for a anything()

61

no need for allOf, all those matchers are variadic...

65–66

I think that hasRHS(ignoringImpCasts(CallExpression)) should do a trick.

96

This is not a Decl but an Expr

155–156

Maybe MatchedDecl->getSourceRange () ?

clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h
61

no need for mutable, its used only form Check that isn't const.
Maybe better HasIsPresentCache ?

@PiotrZSL Thank you for your review, however we tend to only comment on code that has been changed by the patch. You have made some good points about parts of this check, but they can be addressed in an NFC commit.

clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h
61

Good spot.

PiotrZSL resigned from this revision.Apr 10 2023, 11:09 PM
carlosgalvezp added inline comments.Apr 23 2023, 3:15 AM
clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h
61

Comment not yet addressed, do you intend to?

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

Would it make more sense to point to an actual commit in the repo? A review may or may not have been merged.

clang-tools-extra/test/clang-tidy/checkers/llvm/prefer-isa-or-dyn-cast-in-conditionals.cpp
1

Typo