This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Add new checker: llvm-prefer-isa-or-dyn-cast-in-conditionals
ClosedPublic

Authored by hintonda on Mar 25 2019, 3:19 PM.

Details

Summary

Looks at conditionals and finds cases of `cast<>`, which will
assert rather than return a null pointer, and `dyn_cast<>` where
the return value is not captured. Additionally, finds cases that
match the pattern `var.foo() && isa<X>(var.foo())`, where the
method is called twice and could be expensive.

.. code-block:: c++

// Finds cases like these:
if (auto x = cast<X>(y)) <...>
if (cast<X>(y)) <...>

// But not cases like these:
if (auto f = cast<Z>(y)->foo()) <...>
if (cast<Z>(y)->foo()) <...>

Event Timeline

hintonda created this revision.Mar 25 2019, 3:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2019, 3:19 PM
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21

Please use using. See modernize-use-using.

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

Please enclose cast<> in ``.

clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
7

Please enclose cast<> in ``. Same below.

8

Please fix double space.

10

Please fix double dot.

Eugene.Zelenko edited projects, added Restricted Project; removed Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptMar 25 2019, 3:23 PM
hintonda marked an inline comment as done.Mar 25 2019, 3:30 PM
hintonda added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21

Normally I'd agree, but since the llvm checkers live in the llvm namespace, including any llvm checker prior to this header will prevent clang from finding llvm::SmallSet. As you can see, the other llvm classes don't need the llvm:: prefix, but that's because clang/include/clang/Basic/LLVM.h already has a bunch of using statements.

So, this code won't compile without this change, or the addition of using llvm::SmallSet; to clang/include/clang/Basic/LLVM.h. Should I make that change first? Then just remove the llvm:: prefix?

hintonda marked an inline comment as done.Mar 25 2019, 3:46 PM
hintonda added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21

Oh, sorry, didn't read closely enough. I can get rid of the typedef.

hintonda updated this revision to Diff 192234.Mar 25 2019, 6:16 PM
  • Address comments.
Eugene.Zelenko added inline comments.Mar 25 2019, 6:19 PM
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
17

It'll be easier to use llvm::SmallSet in using statement.

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

Please use double, not single `.

hintonda marked an inline comment as done.Mar 25 2019, 6:31 PM
hintonda added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
17

Unfortunately, that won't compile.

The reason is that the llvm checkers live in namespace clang::clang-tidy::llvm, so if you include an llvm checker header, which opens the clang::clang-tidy::llvm namespace, clang doesn't know whether to look in llvm or clang::clang-tidy::llvm since you are in clang::clang-tidy.

The error does suggest using ::llvm::SmallSet, but that seems sorta klugey.

Since this looks ugly, how about I add using llvm::SmallSet; to clang/include/clang/LLVM.h?

hintonda marked an inline comment as done.Mar 25 2019, 7:05 PM
hintonda added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
17

Btw, the reason this never caused a problem before was that the first llvm checker header included in clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp was HeaderGuardCheck.h, which only include HeaderGuardCheck.h before opening the new clang::clang-tidy::llvm namespace. Once I added this checker, which was included before HeaderGuardCheck.h, it made the new llvm namespace was ambiguous within clang-tidy, hence the error.

hintonda updated this revision to Diff 192238.Mar 25 2019, 7:42 PM
  • Address additional comments, and move to LLVM.h.

Should this check also try to find this pattern:

if (dyn_cast<Frobble>(Bobble))
  ...

in order to recommend the proper replacement:

if (isa<Frobble>(Bobble))
  ...

I ask because the name llvm-avoid-cast-in-conditional sounds like it would also cover this situation and I run into it during code reviews with some frequency (more often than I run into cast<> being used in a conditional).

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
21

No need to register this matcher if C++ isn't enabled.

36–37

clang-tidy diagnostics should not be complete sentences; you should make this ungrammatical. How about: cast<> in conditional will assert rather than return a null pointer?

38

Spurious semicolon?

clang-tools-extra/docs/clang-tidy/checks/llvm-avoid-cast-in-conditional.rst
9

This cast<> still needs to be enclosed in ``.

Should this check also try to find this pattern:

if (dyn_cast<Frobble>(Bobble))
  ...

in order to recommend the proper replacement:

if (isa<Frobble>(Bobble))
  ...

I ask because the name llvm-avoid-cast-in-conditional sounds like it would also cover this situation and I run into it during code reviews with some frequency (more often than I run into cast<> being used in a conditional).

Yes, I can add that, and provide a fix-it too. Thanks...

Should this check also try to find this pattern:

if (dyn_cast<Frobble>(Bobble))
  ...

in order to recommend the proper replacement:

if (isa<Frobble>(Bobble))
  ...

I ask because the name llvm-avoid-cast-in-conditional sounds like it would also cover this situation and I run into it during code reviews with some frequency (more often than I run into cast<> being used in a conditional).

Yes, I can add that, and provide a fix-it too. Thanks...

I did a quick grep and found a few of these, but also found the _or_null<> variety. Guess they'll need to stay the same since isa<> can't handle nulls.

Btw, I also found the same pattern used for while(), so I'll add that too. Here's a sample of the patterns I'm seeing:

./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213: while (dyn_cast<NullStmt>(last_stmt)) {
./clang/lib/CodeGen/CodeGenModule.cpp:1390: if (dyn_cast_or_null<NamedDecl>(D)) . // <--- this one's okay
./clang/lib/CodeGen/CodeGenModule.cpp:3950: if (dyn_cast<llvm::CallInst>(callSite)) {

Should this check also try to find this pattern:

if (dyn_cast<Frobble>(Bobble))
  ...

in order to recommend the proper replacement:

if (isa<Frobble>(Bobble))
  ...

I ask because the name llvm-avoid-cast-in-conditional sounds like it would also cover this situation and I run into it during code reviews with some frequency (more often than I run into cast<> being used in a conditional).

Yes, I can add that, and provide a fix-it too. Thanks...

I did a quick grep and found a few of these, but also found the _or_null<> variety. Guess they'll need to stay the same since isa<> can't handle nulls.

Would it make sense to transform if (dyn_cast_or_null<T>(Obj)) into if (Obj && isa<T>(Obj)) or are there bad transformations from that?

Btw, I also found the same pattern used for while(), so I'll add that too. Here's a sample of the patterns I'm seeing:

./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213: while (dyn_cast<NullStmt>(last_stmt)) {

Hah, good catch!

./clang/lib/CodeGen/CodeGenModule.cpp:1390: if (dyn_cast_or_null<NamedDecl>(D)) . // <--- this one's okay

I think this could be expressed as if (D && isa<NamedDecl>(D)), no?

Would it make sense to transform if (dyn_cast_or_null<T>(Obj)) into if (Obj && isa<T>(Obj)) or are there bad transformations from that?

Sure, that sounds reasonable. I only see about 12 cases across all repos, so it isn't that common. Whereas the idiom you present is used quite often.

I haven't looked yet, but wouldn't the use of isa_or_null<> be more efficient in cases like this?

./clang/lib/Sema/AnalysisBasedWarnings.cpp:401: if (B->getTerminator() && isa<CXXTryStmt>(B->getTerminator()))
./clang/lib/AST/Expr.cpp:2734: if (MCE->getMethodDecl() && isa<CXXConversionDecl>(MCE->getMethodDecl()))

Granted, there aren't a lot of these.

Btw, I also found the same pattern used for while(), so I'll add that too. Here's a sample of the patterns I'm seeing:

./lldb/source/Plugins/ExpressionParser/Clang/ASTResultSynthesizer.cpp:213: while (dyn_cast<NullStmt>(last_stmt)) {

Hah, good catch!

./clang/lib/CodeGen/CodeGenModule.cpp:1390: if (dyn_cast_or_null<NamedDecl>(D)) . // <--- this one's okay

I think this could be expressed as if (D && isa<NamedDecl>(D)), no?

hintonda updated this revision to Diff 192370.Mar 26 2019, 3:04 PM
  • Address additional comments.
hintonda updated this revision to Diff 192416.Mar 27 2019, 12:23 AM
  • Added dyn_cast and dyn_cast_or_null, and provided fixit's for all of them.

I looked at the IR generated at -O2, and found that while if (isa<X>(y)) is a modest win over if (dyn_cast<X>(y), if (dyn_cast_or_null<X>(y)) generates exactly the same IR that if(y && isa<X>(y)) does. Also, if y is actually an expression that makes a function call, it's more expensive because it will make the call twice.

So I don't seen any reason to replace dyn_cast_or_null<> in conditionals.

hintonda updated this revision to Diff 192577.Mar 28 2019, 1:29 AM
  • Removed the dyn_cast_or_null replacements.
hintonda updated this revision to Diff 192754.Mar 28 2019, 6:13 PM
  • Add replacement of y && cast<X>(y) with dyn_cast_or_null<X>(y), which is at least as efficient, and can be a big win if y is a function call.
hintonda updated this revision to Diff 192756.Mar 28 2019, 6:17 PM
  • Remove commented code.
Eugene.Zelenko added inline comments.Mar 28 2019, 6:22 PM
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
74

Please don't use auto where type is not obvious.

75

Please don't use auto where type is not obvious.

83

Please don't use auto where type is not obvious.

84

Please don't use auto where type is not obvious.

91

Please don't use auto where type is not obvious.

92

Please don't use auto where type is not obvious.

102

Please don't use auto where type is not obvious.

105

Please don't use auto where type is not obvious.

108

Please don't use auto where type is not obvious.

hintonda marked an inline comment as done.Mar 28 2019, 6:32 PM
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
74

Happy to make the change, but thought the get.*Loc() methods were obvious?

It's good idea to follow modernize-use-auto convention without introducing exceptions.

It's good idea to follow modernize-use-auto convention without introducing exceptions.

No worries. I'm just not sure I fully grok them.

hintonda updated this revision to Diff 192761.Mar 28 2019, 6:39 PM
  • Remove spurious auto's.

If the dyn_cast_or_null<> replacement is acceptable, I'll update the documentation and warning message -- suggestions are appreciated.

hintonda updated this revision to Diff 192778.Mar 28 2019, 10:15 PM
  • Add isa and dyn_cast when matching for dyn_cast_or_null replacement.
aaron.ballman marked an inline comment as done.Mar 29 2019, 4:38 AM

I looked at the IR generated at -O2, and found that while if (isa<X>(y)) is a modest win over if (dyn_cast<X>(y), if (dyn_cast_or_null<X>(y)) generates exactly the same IR that if(y && isa<X>(y)) does. Also, if y is actually an expression that makes a function call, it's more expensive because it will make the call twice.

So I don't seen any reason to replace dyn_cast_or_null<> in conditionals.

Mostly because I think it's more clear with isa<> because that's really what it's checking. However, I could see not doing an automatic transform in the case where the expression argument is something expensive, like a function call. I could also see this as an impetus for adding isa_or_null<> as a separate commit.

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
39

This seems like a fair amount of code duplication -- can this be cleaned up using some local variables for the inner matchers?

61

I think the expr() matcher is too general. This will trigger on something like if (foo && isa<T>(bar)) won't it?

I'd also like to see tests for code like:

foo && cast<T>(foo)->bar();

I don't think this pattern should diagnose because you cannot safely replace it with dyn_cast<T>(foo)->bar()

63–65

I do not think we should diagnose code that uses obj && isa<T>(obj) -- that's very reasonable code.

74

Generally, no. There are a fair number of different location types and it's not obvious which type you're getting. We usually mean obvious as in "this type is an iterator, but the concrete type of the iterator isn't that important" or "the type is exactly spelled out as a template argument".

91–92

Should we be concerned about fix-it interactions with macros (here and elsewhere in the check)? We usually suppress fix-its if the replacement range is somewhere within a macro expansion.

97

What if the reason we matched was because the cast was written using a macro of a different name? e.g., #define LONGER_IDENTIFIER(T, Obj) cast<T>(Obj)

113–117

Can this be handled at the matcher level? IIRC we have isExpansionInFileMatching() or something like that.

146

This diagnostic doesn't tell the user what they've done wrong with the code or why this is a better choice.

aaron.ballman added inline comments.Mar 29 2019, 4:38 AM
clang-tools-extra/clang-tidy/tool/run-clang-tidy.py
256–258 ↗(On Diff #192778)

Is this related to your patch? If so, why is it needed?

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21

Personally, I prefer using ::llvm::SmallSet to this change. It makes it very clear what namespace the type lives in.

hintonda updated this revision to Diff 192844.Mar 29 2019, 9:15 AM
hintonda marked 3 inline comments as done.
  • Address comments.

I looked at the IR generated at -O2, and found that while if (isa<X>(y)) is a modest win over if (dyn_cast<X>(y), if (dyn_cast_or_null<X>(y)) generates exactly the same IR that if(y && isa<X>(y)) does. Also, if y is actually an expression that makes a function call, it's more expensive because it will make the call twice.

So I don't seen any reason to replace dyn_cast_or_null<> in conditionals.

Mostly because I think it's more clear with isa<> because that's really what it's checking. However, I could see not doing an automatic transform in the case where the expression argument is something expensive, like a function call. I could also see this as an impetus for adding isa_or_null<> as a separate commit.

My last patch only changes if(y && isa<X>(y)) if y is a CXXMemberCallExpr, so I hope that addresses your concern.

clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
146

Yes, but I'm not yet sure what it should say. Was sorta hoping for a suggestion.

hintonda updated this revision to Diff 192854.Mar 29 2019, 10:16 AM
  • Improve warning message.
hintonda updated this revision to Diff 192939.Mar 29 2019, 4:18 PM
  • Added matcher, and reordered tests.
  • Added matcher, and reordered tests.

Commandline ate my ticks... ;-(

Should be added isMacroID matcher.

aaron.ballman added inline comments.Apr 1 2019, 6:29 AM
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
39

I'm still hoping that some of this duplication can be reduced, btw.

104–105

Spurious code that can be removed?

119

Aside from the fix-it, this code is identical to the block above. Can be combined.

126–127

There are zero test cases that seem to trigger this diagnostic text.

146

Do you have any evidence that this situation happens in practice? I kind of feel like this entire branch could be eliminated from this patch unless it actually catches problems that happen.

clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21

Can you add back the ::llvm:: qualifier on the StringRef type?

clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
23

Please spell out the full diagnostic the first time it occur in the test case - it's fine to abbreviate subsequent diagnostics, but we like to see at least one exact match per diagnostic.

49

Same here.

65

and here

hintonda marked an inline comment as done.Apr 1 2019, 7:31 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21

I could do that, but do you mean just this case of StringRef, of all four? And what about SourceLocation and SourceManager?

aaron.ballman added inline comments.Apr 1 2019, 7:36 AM
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21

If any changes are needed in this file at all, I'd prefer to keep them minimal and self-consistent, so only this instance. However, I wouldn't be opposed to a consistency cleanup in this file as a separate patch with NFC.

hintonda marked an inline comment as done.Apr 1 2019, 8:52 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/utils/HeaderFileExtensionsUtils.h
21

Since fixing the bug in this file is orthogonal to this patch, I'll address it in it's own patch.

hintonda marked an inline comment as done.Apr 1 2019, 10:36 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
126–127

Sorry for any confusion, but there are actually 3 tests for this below. That'll be made clearer once I address your comments below, and spell out the entire diagnostic message on the first instance.

Do we have any checker to recommend llvm functions over std ?

e.g. llvm::sort, llvm::all_of, ...

Do we have any checker to recommend llvm functions over std ?

e.g. llvm::sort, llvm::all_of, ...

No. Actually LLVM coding conventions support is very limited. See https://clang.llvm.org/extra/clang-tidy/checks/list.html.

I think check for auto usage is useful too.

hintonda marked an inline comment as done.Apr 1 2019, 12:34 PM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
146

Yes, here are a few from clang/lib -- let me know if you think it's worth it or not to keep this:

  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 305293 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp Message: method 'getAsTemplateDecl' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp Length: 93 Offset: 305293 ReplacementText: dyn_cast_or_null<TemplateTemplateParmDecl>(Name.getAsTemplateDecl())
  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 153442 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp Message: method 'getAsTemplateDecl' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/ASTContext.cpp Length: 92 Offset: 153442 ReplacementText: dyn_cast_or_null<TypeAliasTemplateDecl>(Template.getAsTemplateDecl())
  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 97556 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp Message: method 'getMethodDecl' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/AST/Expr.cpp Length: 68 Offset: 97556 ReplacementText: dyn_cast_or_null<CXXConversionDecl>(MCE->getMethodDecl())
  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 301950 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp Message: method 'get' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaInit.cpp Length: 49 Offset: 301950 ReplacementText: dyn_cast_or_null<InitListExpr>(CurInit.get())
  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 14335 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp Message: method 'operator bool' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp Length: 57 Offset: 14335 ReplacementText: dyn_cast_or_null<CXXTryStmt>(B->getTerminator())
  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 15997 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp Message: method 'operator bool' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/AnalysisBasedWarnings.cpp Length: 55 Offset: 15997 ReplacementText: dyn_cast_or_null<CXXTryStmt>(B.getTerminator())
  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 9492 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Message: method 'sexpr' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Length: 39 Offset: 9492 ReplacementText: dyn_cast_or_null<til::Undefined>(sexpr())
  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 9572 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Message: method 'sexpr' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Length: 38 Offset: 9572 ReplacementText: dyn_cast_or_null<til::Wildcard>(sexpr())
  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 9492 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Message: method 'sexpr' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Length: 39 Offset: 9492 ReplacementText: dyn_cast_or_null<til::Undefined>(sexpr())
  • DiagnosticName: llvm-avoid-cast-in-conditional FileOffset: 9572 FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Message: method 'sexpr' is called twice and could be expensive Replacements:
    • FilePath: /Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/include/clang/Analysis/Analyses/ThreadSafetyCommon.h Length: 38 Offset: 9572 ReplacementText: dyn_cast_or_null<til::Wildcard>(sexpr())
hintonda updated this revision to Diff 193176.Apr 1 2019, 1:57 PM
  • Defer HeaderFileExtionsUtils.h bugfix to separate patch.
  • Simplify code per comments.
hintonda marked an inline comment as done.Apr 2 2019, 5:30 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
19

@aaron.ballman: This matcher seems genuinely useful. What do you think about moving it to ASTMatchers.h?

hintonda updated this revision to Diff 193376.Apr 2 2019, 3:26 PM
  • Renamed and updated docs.
  • Fix formatting.
hintonda retitled this revision from [clang-tidy] Add new checker: llvm-avoid-cast-in-conditional to [clang-tidy] Add new checker: llvm-prefer-isa-or-dyn-cast-in-conditionals.Apr 2 2019, 3:28 PM
hintonda edited the summary of this revision. (Show Details)
hintonda edited the summary of this revision. (Show Details)
Eugene.Zelenko added inline comments.Apr 2 2019, 3:30 PM
clang-tools-extra/docs/clang-tidy/checks/llvm-prefer-isa-or-dyn-cast-in-conditionals.rst
6 ↗(On Diff #193376)

Indentation is not needed here. Same for following lines.

hintonda updated this revision to Diff 193379.Apr 2 2019, 3:37 PM
  • Remove spaces in docs.
aaron.ballman added inline comments.Apr 3 2019, 5:31 AM
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
19

I think that adding something like this might be a good idea. We don't have any notion of source locations in the AST matching syntax currently, and I'm not certain whether that's a good thing or not here. I'm mildly uncomfortable that this matcher operates on an Expr but then internally uses a source location from that expression and I wonder if we would rather introduce source location matching. For instance, what if the user cares about the difference between getExprLoc() and getBeginLoc() for some reason?

aaron.ballman added inline comments.Apr 3 2019, 5:44 AM
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
146

/Users/dhinton/projects/llvm_project/monorepo/llvm-project/clang/lib/Sema/SemaTemplate.cpp Message: method 'getAsTemplateDecl' is called twice and could be expensive Replacements:

This one is a false positive and the fix is incorrect. The original code is:

Diag(TemplateNameLoc, diag::err_not_class_template_specialization)
  << (Name.getAsTemplateDecl() &&
      isa<TemplateTemplateParmDecl>(Name.getAsTemplateDecl()));

The rest look to be plausible true positives, but not a single case looks like there is much expense involved in the expression, and honestly, several of the replacement expressions would feel a bit "worse" to me, despite being equivalent. For instance, code like: bool Foo = bar && isa<T>(bar); reads more clearly to me than bool Foo = dyn_cast_or_null<T>(bar); because the former looks like a bool initializer while the latter looks like the user meant to write T *Foo instead of bool Foo.

I think this part of the check is contentious and I'd rather leave it out for now so that we can get the rest of the check in. I'd be especially curious to know whether the community would prefer to see us leave this situation alone, introduce isa_or_null<T>(), or use dyn_cast_or_null<T>().

hintonda marked an inline comment as done.Apr 3 2019, 7:29 PM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
146

How is that a false positive? In

Diag(TemplateNameLoc, diag::err_not_class_template_specialization)
  << (Name.getAsTemplateDecl() &&
      isa<TemplateTemplateParmDecl>(Name.getAsTemplateDecl()));

Name.getAsTemplateDecl() is called twice. First to see if returned nullptr or not, then to see if the pointer satisfied isa<TemplateTemplateParmDecl>(Name.getAsTemplateDecl()).

As for whether or not it's worth having, I'll defer to you. Most cases are relatively cheap, but it can be expensive. I'll go back and rerun it on all of clang+llvm and find the expensive ones if you're interested, but it takes a really long time to run on my laptop, so let me know what you'd prefer.

hintonda marked an inline comment as done.Apr 3 2019, 7:39 PM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
146

Btw, I'll draft a message to the list and ask about isa_or_null, et al, in the morning. I didn't want to add stuff like that in this patch, but would be happy to use it if I got the green light to add it in another.

Thanks again for the feedback -- especially on the matchers.

hintonda marked an inline comment as done.Apr 3 2019, 7:51 PM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
19

Well, you can attach it to whatever you want, so I'm not sure that's a problem. Alternatively, you have to check each location yourself. In my case, that was multiple places, so putting it in the matcher cleaned up the code.

I need to verify it, but it seems that it triggered when any macros were in the range, but I need to look closer into that to understand the behavior. I'll check it out and get back to you.

aaron.ballman added inline comments.Apr 4 2019, 4:54 AM
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
146

How is that a false positive?

Sorry! You are correct, I was being imprecise. I meant that the fix here is definitely bad and it demonstrates one of the things I was concerned about for the check. I also meant that the diagnostic firing here is suggesting that this code is wrong when in fact, naively conforming to the check would make the code incorrect. One possible solution to this is for the check to also include static_cast<bool>(<expr>), but I'm not convinced that's great either.

As for whether or not it's worth having, I'll defer to you. Most cases are relatively cheap, but it can be expensive. I'll go back and rerun it on all of clang+llvm and find the expensive ones if you're interested, but it takes a really long time to run on my laptop, so let me know what you'd prefer.

I went through all of the places in the list you posted above and did not see a single case where this would make a lot of difference in terms of performance. In every case it's a function call to a simple accessor, effectively.

146

Btw, I'll draft a message to the list and ask about isa_or_null, et al, in the morning. I didn't want to add stuff like that in this patch, but would be happy to use it if I got the green light to add it in another.

Yup, agreed! I think the best approach right now is to see if isa_or_null<> gets some traction before moving forward with this part of the patch. You can either stall this patch until that discussion looks like it's getting consensus, or you can remove the functionality from this patch and land it sooner, then add it back in if isa_or_null<> gets adopted.

Thanks again for the feedback -- especially on the matchers.

My pleasure!

aaron.ballman added inline comments.Apr 4 2019, 4:59 AM
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
19

Well, you can attach it to whatever you want, so I'm not sure that's a problem.

How does a caller of this AST matcher indicate that they want it to match on Node.getBeingLoc().isMacroID() instead of what's written now? If the answer is "they write a different matcher", then that's what I think could be a problem. There's a lot of ways to get a source location (and a lot of different kinds of source locations), and I think matching on those can be very useful functionality. So I think the idea of the matcher is good, I'm just not certain whether we want it to look like AST_MATCHER(Expr, isMacroID); or AST_MATCHER(SourceLocation, isMacroID); or something else.

hintonda marked an inline comment as done.Apr 4 2019, 7:47 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/AvoidCastInConditionalCheck.cpp
19

I'll workup a separate patch with tests and examples, and see if I can allay your concerns.

hintonda updated this revision to Diff 193968.Apr 5 2019, 2:30 PM
  • Use new isa_and_nonnull, and cleanup code.
hintonda updated this revision to Diff 194017.Apr 6 2019, 7:14 AM

Explicitly check for implicitCastExpr and add additional negative matching test.

aaron.ballman added inline comments.Apr 11 2019, 9:27 AM
clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.cpp
125 ↗(On Diff #194017)

How about 'isa_and_nonnull<>' is preferred over an explicit test for null followed by calling 'isa<>'?

clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h
37–39 ↗(On Diff #194017)

I think this comment is now out of date.

hintonda marked an inline comment as done.Apr 11 2019, 4:08 PM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/PreferIsaOrDynCastInConditionalsCheck.h
37–39 ↗(On Diff #194017)

Thanks, I'll fix that.

I'll give the RFC a few more days, but not sure it's been definitive. A few no votes, but there are always some of those. I sorta like isa_nonnull<T> for brevity, so I'll mention that why I follow-up on that thread. Btw, the isa_and_nonnull<T> has been in for a week: r357761, and nothing on that thread leads me to believe I should revert it.

hintonda updated this revision to Diff 194782.Apr 11 2019, 4:22 PM
  • Update isa_and_nonnull diagnostic message, and fix typo.
aaron.ballman accepted this revision.Apr 15 2019, 12:42 PM

LGTM aside from a nit and the ultimate name for the isa_and_nonnull<> API.

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
29–30

Please keep this list sorted alphabetically.

This revision is now accepted and ready to land.Apr 15 2019, 12:42 PM
hintonda marked an inline comment as done.Apr 15 2019, 2:00 PM

LGTM aside from a nit and the ultimate name for the isa_and_nonnull<> API.

Thanks. I'll ping the list again at the end of the week, then make whatever change is necessary.

Btw, the current iteration replaces all v && isa<T>(v) instances with isa_and_nonnull<T>(v). Is that still okay, or should it just do method calls?

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
29–30

I think this is where add_new_check.py put it, but I'll alphabetize.

hintonda updated this revision to Diff 195270.Apr 15 2019, 4:42 PM
  • Alphabetize registration calls.

LGTM aside from a nit and the ultimate name for the isa_and_nonnull<> API.

Thanks. I'll ping the list again at the end of the week, then make whatever change is necessary.

Btw, the current iteration replaces all v && isa<T>(v) instances with isa_and_nonnull<T>(v). Is that still okay, or should it just do method calls?

I think it's fine to replace all instances. We can relax the check if it seems like onerous behavior in practice.

clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
29–30

Huh, I thought that script kept everything alphabetized? That may be worth investigating (don't feel obligated though).

hintonda marked an inline comment as done.Apr 16 2019, 7:19 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
29–30

I'm sure it's something I did inadvertently, but can't really remember the details.

hintonda marked an inline comment as done.Apr 17 2019, 8:40 AM
hintonda added inline comments.
clang-tools-extra/clang-tidy/llvm/LLVMTidyModule.cpp
29–30

Looks like rename_check.py definitely does not alphabetize. I'll try to address than in another patch. Thanks for pointing this out.

@aaron.ballman, I just ran it over llvm/lib, including all in-tree headers, and it seems to work fine. However, it did miss this one:

  • if (V && isa<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

+ if (isa_and_nonnull<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

It got the first, but not the second. Not sure how to pick that one up. Even ran it a second time on just that file, but still didn't pick it up. Any ideas?

@aaron.ballman, I just ran it over llvm/lib, including all in-tree headers, and it seems to work fine. However, it did miss this one:

  • if (V && isa<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

+ if (isa_and_nonnull<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

It got the first, but not the second. Not sure how to pick that one up. Even ran it a second time on just that file, but still didn't pick it up. Any ideas?

I don't think it's a critical case to cover for the check, but yeah, it looks like that code really wants to be (EntInst = dyn_cast_or_null<Instruction>(V)). I think that looking for a pattern to handle this case would be tricky though and given how infrequent it seems to show up in the code base, I am not too worried. Someone applying the check will still get a notice for the Var && isa<Type>(Var) part of the expression, so they can hopefully see there's more related cleanup to be done.

@aaron.ballman, I just ran it over llvm/lib, including all in-tree headers, and it seems to work fine. However, it did miss this one:

  • if (V && isa<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

+ if (isa_and_nonnull<Instruction>(V) && (EntInst = cast<Instruction>(V)) &&

It got the first, but not the second. Not sure how to pick that one up. Even ran it a second time on just that file, but still didn't pick it up. Any ideas?

I don't think it's a critical case to cover for the check, but yeah, it looks like that code really wants to be (EntInst = dyn_cast_or_null<Instruction>(V)). I think that looking for a pattern to handle this case would be tricky though and given how infrequent it seems to show up in the code base, I am not too worried. Someone applying the check will still get a notice for the Var && isa<Type>(Var) part of the expression, so they can hopefully see there's more related cleanup to be done.

Okay, thanks for looking. I'll go ahead and land this today, and if I think of a good way to handle this case, I'll update it later.

Thanks again for all your help...

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 24 2019, 2:25 PM