[llvm] [Support][Casting] Add predicates for `isa*` functions (PR #83753)

David Blaikie via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 5 12:14:23 PST 2024


dwblaikie wrote:

> > > * Any code that explicitly specified the `From` template argument would **silently** break (the interpretation would change to another possible cast type)
> > 
> > 
> > Hmm, not sure I follow this - I'd expect the functor to have one template parameter, so anyone explicitly specifying 2 would fail to compile at least...
> 
> @dwblaikie Currently, `isa` is defined like this:
> 
> https://github.com/llvm/llvm-project/blob/ee044d5e651787c5d73b37b2cbb7ca6444bb0502/llvm/include/llvm/Support/Casting.h#L547-L555
> 
> I'm worried that the meaning of `&isa</*To=*/T1, /*From=*/T2>` would change to `&isa</*To1=*/ T1, /*To2=*/T2>` in the context of something like:
> 
> ```c++
> auto fPtr = &isa<VectorType, Type>;
> ...
> bool isA = (*fPtr)(ptr);
> ```

Hmm, sorry, not really following... I don't think we'd support multiple target types... it'd have one template parameter, the target type, and specifying 2 would fail to compile. (I think the ability to specify the from type is sort of incidental/accident of the template implementation, not an intentional feature of isa/cast/etc)

but it's probably irrelevant, since, as you say, disabling ADL means functor solutions aren't likely ideal/feasible for backwards compatibility reasons.

> Open to discussion here - but I'm inclined to think the ability specify the second template parameter to isa is a bug/incidental feature of isa being a function template, not a feature & one we could choose not to expose in these predicates (by only providing one template parameter, rather than the parameter pack on the outer functor class)? Like if someone wants to use a specific argument type - they should convert the argument expression themselves, rather than by specifying a second template argument and then relying on impliict conversion of the function parameter?

Still be curious about this ^ but a relatively minor issue.



https://github.com/llvm/llvm-project/pull/83753


More information about the llvm-commits mailing list