[PATCH] D102369: [ASTMatchers][NFC] Remove runtime checks where compile time is sufficient

Thu May 13 04:37:40 PDT 2021

aaron.ballman added a comment.

In D102369#2756516 <https://reviews.llvm.org/D102369#2756516>, @njames93 wrote:

> In D102369#2756493 <https://reviews.llvm.org/D102369#2756493>, @aaron.ballman wrote:
>
>> I'm not opposed, but I don't know that this is really an improvement. We've gone from a pretty simple code pattern to needing to spell out the type twice with a type_trait, and all that we save is a call to `isa<>` within the casting operation. While I agree this is strictly executing less code, it's morally the same as the usual antipattern of "isa followed by cast should be a dyn_cast". Is there evidence that this is a big performance win?
>
> To be honest, it would be much nicer if we could specialise `isa` to return false if the `From` type is not a base class of the `To` type. However I'm not sure if that would break anything else.

That might be interesting to explore.

> It's all part of the "Don't do at runtime whats known at compile time" idiom. Any performance change would be negligible in this case.

If that's the sole reason for the change, I'm not convinced it carries its weight. It's more code, it's more complicated to read, violates the DRY principle, and has negligible performance benefit. However, perhaps @klimek has different opinions as code owner.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D102369/new/

https://reviews.llvm.org/D102369