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

[David Blaikie via llvm-commits]

On Wed, Jan 24, 2018 at 12:37 PM Dmitriy Borisenkov <
dmitriy.borisenkov89 at gmail.com> wrote:

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

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.


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

I'd be happy to rule out using cast for upcasting - it seems like it'd only
make the code harder to read.


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

That's a nice-to-have, for sure, from a generality perspective.

(though more broadly: What's your end goal? Do you have some functionality
you have a particular use case for already?)


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

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.


> 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/69a81254/attachment.html>