[PATCH] D42027: [Support][NFC] Improve isa implementation

[Dmitriy Borisenkov via llvm-commits]

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.

Upcasts in that context are also might make sence. I could imagine
something like
auto * const base = cast_or_null<Base>(derived); // alternatively
[&derived](){if (derived) return derived.getPointer(); return nullptr;}
Though again it was not my intension to write code like this, but probably
it's not that bad code after all.

Another side effect of the patch is shared_ptr support. For isa only by now.

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). 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.

--
Kind regards, Dmitry

On Wed, Jan 24, 2018 at 10:27 PM, David Blaikie <dblaikie at gmail.com> wrote:

> In retrospect is it useful for Optional to be supported in isa? Since its
> dynamic type is the same as its static type?
>
> On Tue, Jan 23, 2018 at 10:50 PM Dmitry Borisenkov via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> dmitry added inline comments.
>>
>>
>> ================
>> Comment at: unittests/Support/Casting.cpp:288-289
>> +  EXPECT_TRUE(isa<Base>(DShared));
>> +  llvm::Optional<Derived> DOpt;
>> +  EXPECT_TRUE(isa<Base>(DOpt));
>> +}
>> ----------------
>> dblaikie wrote:
>> > 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)
>> This change doesn't modify casts behavior - just isa.
>> As for the plan, cast should return T*, so
>> ```
>> Optional<Derived> DOpt;
>> Base *b = cast<Base>(DOpt); //Just cast pointer to underlying data to
>> Base*, does not handle potential ownership issues.
>> ```
>> should work, but it's ok.
>> Move cast would be indeed problematic for value-types. So I would
>> probably restrict it.
>>
>>
>> https://reviews.llvm.org/D42027
>>
>>
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180124/cb76909a/attachment.html>