[PATCH] D123901: [LLVM][Casting.h] Allow dyn_cast to return Optional for types that are not constructble from nullptr.

[Aman LaChapelle via Phabricator via llvm-commits]

bzcheeseman added a comment.

> It may be helpful to also show the patch with the intended use-case.

The intended use case is to be used for projects like MLIR that have `t.dyn_cast<IntegerType>()` so they can instead have `dyn_cast<IntegerType>(t)` for consistency, and to reduce the RTTI burden.

> I expect this is going to cause a lot of confusion when people work on an Instruction &, but really want to be working on Instruction *.

I'm maybe missing something here, but my understanding is that if you have `Value *v; auto instruction = dyn_cast<Instruction>(v);` you're going to end up with an `Instruction *`. Am I reading the enable_if/conditional rules incorrectly?

> can they be merged into a single null propagating thing?

I like this idea - I originally had dyn_cast_or_null with an overload for Optional but it looked goofy. I like `dyn_cast_if_present` with maybe some forwarding `dyn_cast_or_null` to avoid breaking everyone.

> dyn_cast is such a core operation <>
> ...
> we need to be careful with such a core API like this, it would be good to send a "head's up" note to the llvm forum when the design point settles

Agreed 100%. Everyone uses dyn_cast and that's why I wanted to put up the patch, so we can talk about it!

Going forward I see 2 options (would love to hear about it if someone has other ideas):

1. Keep the overload, maybe swap the `dyn_cast_or_none` for `dyn_cast_if_present`
2. Have a new function `optional_dyn_cast`/`optional_dyn_cast_if_present` (or something, not enamored with that name) so that folks can opt-in to the new behavior with optionals

I would personally prefer option 1 here because it makes the most sense to me right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123901