<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Jan 24, 2018 at 12:37 PM Dmitriy Borisenkov <<a href="mailto:dmitriy.borisenkov89@gmail.com">dmitriy.borisenkov89@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Though in this particar case optional support was just a side effect (and the main purpose was to get rid of 5 specializations for isa_imp_cl with almost identical code), I hope it's not harmful at least. </div></blockquote><div><br>I hope/wouldn't mind if some other folks would chime in on this issue - whether disallowing cast of something like an Optional would be worthwhile, or if it's an acceptable quirk/generality of this code.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Upcasts in that context are also might make sence. I could imagine something like<br></div><div><div><div>auto * const base = cast_or_null<Base>(derived); // alternatively [&derived](){if (derived) return derived.getPointer(); return nullptr;}</div></div><div>Though again it was not my intension to write code like this, but probably it's not that bad code after all.</div></div></div></blockquote><div><br>I'd be happy to rule out using cast for upcasting - it seems like it'd only make the code harder to read.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div>Another side effect of the patch is shared_ptr support. For isa only by now.<br></div></div></blockquote><div><br>That's a nice-to-have, for sure, from a generality perspective.<br><br>(though more broadly: What's your end goal? Do you have some functionality you have a particular use case for already?)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div></div><div>Probably it makes sense for me to proceed with casts implementation to see if I could really make them more generic and clean because isa is almost useless without casts (so you could consider the current patch as refactoring only).</div></div></blockquote><div><br>Almost, but not entirely useless - some code just does type checking & different custom handling, etc. So I don't have a fundamental problem with this as a standalone patch as such.<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div> Then we could return back to the question if unified behavior for cast usefull and if there are any corner cases when it would break programmers' expectations. If you think it might be usefull, that's enough for me at the moment. I posted the unfinished work to check if it sensible at all or am I just wasting time.<br></div><div><br></div><div>--</div><div>Kind regards, Dmitry</div></div><div dir="ltr"><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jan 24, 2018 at 10:27 PM, David Blaikie <span dir="ltr"><<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">In retrospect is it useful for Optional to be supported in isa? Since its dynamic type is the same as its static type?</div><div class="m_-1168216130263488819HOEnZb"><div class="m_-1168216130263488819h5"><br><div class="gmail_quote"><div dir="ltr">On Tue, Jan 23, 2018 at 10:50 PM Dmitry Borisenkov via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">dmitry added inline comments.<br>
<br>
<br>
================<br>
Comment at: unittests/Support/Casting.cpp:288-289<br>
+  EXPECT_TRUE(isa<Base>(DShared));<br>
+  llvm::Optional<Derived> DOpt;<br>
+  EXPECT_TRUE(isa<Base>(DOpt));<br>
+}<br>
----------------<br>
dblaikie wrote:<br>
> Does this change then mean that one could cast<T> from Optional<Derived> to Optional<Base>? Because that'd be problematic, owing to Optional being a value-type. (it'd slice the object)<br>
This change doesn't modify casts behavior - just isa.<br>
As for the plan, cast should return T*, so<br>
```<br>
Optional<Derived> DOpt;<br>
Base *b = cast<Base>(DOpt); //Just cast pointer to underlying data to Base*, does not handle potential ownership issues.<br>
```<br>
should work, but it's ok.<br>
Move cast would be indeed problematic for value-types. So I would probably restrict it.<br>
<br>
<br>
<a href="https://reviews.llvm.org/D42027" rel="noreferrer" target="_blank">https://reviews.llvm.org/D42027</a><br>
<br>
<br>
<br>
</blockquote></div>
</div></div></blockquote></div><br></div></div></blockquote></div></div>