Page MenuHomePhabricator

[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()) <...>

Diff Detail

Repository
rL LLVM

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
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 ↗(On Diff #192778)

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
145 ↗(On Diff #192778)

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
103–104 ↗(On Diff #192939)

Spurious code that can be removed?

118 ↗(On Diff #192939)

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

125–126 ↗(On Diff #192939)

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

38 ↗(On Diff #192778)

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

145 ↗(On Diff #192778)

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 ↗(On Diff #192939)

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

clang-tools-extra/test/clang-tidy/llvm-avoid-cast-in-conditional.cpp
22 ↗(On Diff #192939)

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.

48 ↗(On Diff #192939)

Same here.

64 ↗(On Diff #192939)

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 ↗(On Diff #192939)

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 ↗(On Diff #192939)

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 ↗(On Diff #192939)

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
125–126 ↗(On Diff #192939)

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
145 ↗(On Diff #192778)

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
18 ↗(On Diff #193176)

@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
18 ↗(On Diff #193176)

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
145 ↗(On Diff #192778)

/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
145 ↗(On Diff #192778)

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
145 ↗(On Diff #192778)

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
18 ↗(On Diff #193176)

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
145 ↗(On Diff #192778)

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.

145 ↗(On Diff #192778)

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
18 ↗(On Diff #193176)

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
18 ↗(On Diff #193176)

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
27–28 ↗(On Diff #194782)

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
27–28 ↗(On Diff #194782)

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
27–28 ↗(On Diff #194782)

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
27–28 ↗(On Diff #194782)

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
27–28 ↗(On Diff #194782)

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