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

[Chris Lattner via Phabricator via llvm-commits]

lattner added a comment.

> Not a big fan of this overload. dyn_cast is such a core operation, I'd rather not muddy things by making it return Optional<> -- but only sometimes.

I agree with you that we need to tread lightly here, but dyn_cast being such a core operation means that we need to continue to invest in it.  There are a bunch of cases that it doesn't handle well, which means LLVM projects using this (e.g. MLIR) end up resorting to similar-but-different things, like `x.dyn_cast<foo>()` which have similar but different behavior on null.  It would be great for the project to pull this together.

> // These three functions provide forwarding for non-optional types passed into
> // dyn_cast_or_none.

I don't understand the addition of the "or_none" helper.  I get what it "does": it allows none propagation for Optional arguments (but it also works with nullptr propagation from pointers as well).  In practice, this seems strictly stronger than the existing dyn_cast_or_null function - can they be merged into a single null propagating thing?  Maybe we should rename it to "dyn_cast_if_present" or some other name that is more generic?

Channeling the comments above, 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.

-Chris


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123901