[PATCH] D33125: Introduce isoneof<T0, T1, ...> as an extension of isa<T>

Mehdi AMINI via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 1 09:34:39 PDT 2017


mehdi_amini added a comment.

In https://reviews.llvm.org/D33125#769930, @chandlerc wrote:

> Honestly, the more I think about this, the more I think that the complexity it involves doesn't pull its weight.


I'm fairly neutral on this.

> First, we either have to deal with the semantic confusion of `isa<T0, T1, T2, ...>` or the readability burden of inventing and using a somewhat terrible name like `isa_anyof`

For a non-native speaker, I still don't understand why the correct form isn't `is_anyof` ( "is any of")? You may explain this to me tonight :)

> or the technical complexity of magical tag templates that are *unqualified* and jammed in here for `isa<anyof<...>>`. Note that we have `llvm::any_of` already and it is very different! =[ None of these are insurmountable issues, but they add cost to this abstraction.

I thought exactly about these after the last update!

> Second, the benefit is relatively small. Until the list of types is loooooong, `isa<T0> || isa<T1> || isa<T2>` really doesn't seem that bad. It's a bit more characters to type, but it neatly solves any and all of the semantic / syntactic ambiguity with zero complex implementation. Kinda nice.

Well I can't refrain but remembering all the people that didn't want to use the STL algorithm and write raw for loop using this same argument.
What's missing is that "any_of" express clearly the intent and is a lot easier to parse for a human. As Serge mentioned in his last example, DRY can be annoying to attain without such construct.

> But even worse, when these lists *do* get long, using `isa`, even with the new tool, is usually *the wrong approach*. People should instead be writing a proper switch over whatever data field is available rather than a series of tests. Once that series is long, I worry will not be reliable.

Reliable on which aspect? Optimization?

> And we have historically seen places where `isa` and `dyn_cast` did show up on the profile.

I see the positive aspect of it: more opportunities to improve compiler optimizations ;)
Driving syntactic changes because of optimizer failure makes me sad...


Repository:
  rL LLVM

https://reviews.llvm.org/D33125





More information about the llvm-commits mailing list