[llvm-commits] [patch] allow ref->pointer through dyn_cast

[David Blaikie]

On Tue, May 1, 2012 at 10:57 AM, Chandler Carruth <chandlerc at google.com> wrote:
> On Tue, May 1, 2012 at 10:39 AM, David Blaikie <dblaikie at gmail.com> wrote:
>>
>> This patch allows reference types to be passed to llvm::dyn_cast -
>> while always returning a pointer (since dyn_cast needs a way to
>> communicate failure to the caller - unlike the standard dynamic_cast
>> on references, which throws). A documentation update is included as
>> well.
>>
>> This provides some flexibility to callers and actually models the
>> contract more appropriately anyway - passing null to dyn_cast results
>> in an assertion, so why not take something that describes that
>> contract syntactically (a reference).
>
>
> I really don't like this:
>
> 1) it makes the behavior non-obvious from inspection: pointer-in,
> pointer-out is a useful invariant.

What other behavior could be implied here? dyn_cast can't accept null
anyway, why would it take a pointer? (though I'm not removing the
dyn_cast that takes a pointer for those that want to use it)

> 2) it breaks symmetry with cast: cast already handles references, but it
> returns a reference

Cast doesn't need to represent failure so its in/out can be symmetric,
dyn_cast's API (non-null pointer in, null pointer out) is already
asymmetric & can be represented more concretely in the type system by
ref in/pointer out.

> 3) it breaks symmetry with dyn_cast_or_null: either this function accepts a
> reference, which makes no sense at all, or it accepts different types

It's a different contract, as demonstrated by the name - it shouldn't
accept a reference, it should be pointer in/pointer out. (in some
mystical world I'd just have dyn_cast on pointers have the "or_null"
behavior and the dyn_cast on references have the new behavior I'm
adding - but we use guaranteed non-null pointers far too pervasively
for that)

> Also, it seems to be solving a non-problem:
>
>>
>> For context, this was motivated
>> by some changes to iterators I've been making in clang - rather than
>> having to dyn_cast(&*i) or make the iterator convertible to T* and
>> provide simplify_type traits (to enable dyn_cast(i)),
>
>
> What's wrong with this? It seems to make everything behave more
> idiomatically...

Gut reaction is that implicit conversion operators can be error prone,
especially conversion to a pointer type (iterators then become
implicitly bool-able (with the value always being 'true') which drops
some amount of type safety when passing function parameters, for
example). Pragmatically, it's also a fair bit of boilerplate for each
of these pseudo-iterators in DeclBase.h - not that I mind the
copy/paste terribly if it produces the right interface, I'm just not
sure iterator->T* is the right interface in general, or the right
thing to rely on in this particular case.

[I realize I'm butting up against an idiomatic, arguably stylistic,
difference of opinion here & I don't mean to use changes as a wedge to
drive some esoteric idiom that's just going to create another
inconsistency in the LLVM codebase. But essentially I'd like to see
less pointers - for me, it makes code easier to read when the contract
is expressed in the type system rather than by convention. If
something can't accept a null pointer, it should take a reference, not
a pointer. This doesn't have to be a global change - but every API
that does follow this convention is, for me at least, marginally
easier to read & use]