LLVM’s isa<> operator was extended to be variadic a few years back. This was primarily driven by MLIR use cases at that time. LLVM code however has mostly been not using that feature, though there are several uses now. However, there was some concern expressed in the PR to adopt this more widely and I was hoping to get some opinions/consensus as to whether we would like to adopt this feature more widely going forward or refrain from using it. Based on the concerns expressed, we could restrict its use to be allowed only in the positive context, i.e., isa<X, Y>(V) and not !isa<X, Y>(V) to not trade readability for conciseness.
As noted on the PR, this seems to be used widely in MLIR code base, so this is really about LLVM codebase allowing broader adoption.
As someone who primarily works in MLIR and MLIR-based codebases, I’m used to seeing variadic isa and it seems quite readable to me. I’d be strong -1 for removing it.
GCC has not added a variadic is_a yet but does have a variadic like TREE_NOT_CHECK* set of macros (originally C, then C++98 but nobody has updated them for C++11 or recently for C++14). So there are use cases in GCC where a variadic !is_a would appear. (GCC is not as agressive of changing code to a C++ style code).
I think we might consider renaming the variadic isa to something like is_any_of, so that its semantics are immediately clear without extra mental effort.
For example, isa<A, B, C>(x) vs. is_any_of<A, B, C>(x) — the latter reads more naturally and requires less cognitive load.
This could also address nikic’s concern IMO (maybe?).
Would a different name solve the concerns, e.g. is_any_of? It seems like that would be easier to mentally expand, i.e. is_any_of<Y, Z>(X) becomes “Is X any of Y or Z”. To also aid readability, I’d then add is_none_of<Y, Z>(X) as the inverse, to avoid the mental gymnastics of applying ! to the result.
I think variadic isa should be adopted more widely. I’m not in favor of renaming to is_any_of/is_none_of, as I think it would create unnecessary code churn.
I agree that is_any_of/is_none_of is more readable than isa with multiple classes. How about this: We add is_any_of and is_none_of as aliases for multiple argument isa (will need 2 or more classes) and document those in the programmer’s manual and maybe do some seed adoption?
That way, existing code continues to work (no need of churn) and for new code it’s preferred to use these new forms over variadic isa. Existing code may be migrated over time as it evolves.
IMO isa is so popular across the codebase that it makes sense to keep the name concise. I’m concerned that adding new names would cause unnecessary churn, either in code reviews is we have two names for the same thing, or through refactors if we decide to only allow the variadic case under the new name.
Instead, I’d start by adding the variadic case to the programmers manual and not introduce a new name.
