llvm dev - May 2019 - [RFC] Should we add isa_or_null<>?

Hi All:

Just wanted to wind this up and summarize the results.

Although there were a few no votes, it looks like there's a consensus for
adding a `isa_and_nonnull` type operator.  While there were some who
preferred `isa_nonnull`, it wasn't overwhelming, and since
`isa_and_nonnull` is already committed, I'm going to leave it as
`isa_and_nonnull` for the time being.

Thanks for all the comments.
don



On Thu, Apr 11, 2019 at 7:49 PM Don Hinton <hintonda at gmail.com> wrote:
> On Wed, Apr 10, 2019 at 11:37 AM David Greene <dag at cray.com>
wrote:
>
>> Don Hinton via llvm-dev <llvm-dev at lists.llvm.org> writes:
>>
>> > Used like this:  isa<T,1>(v) or isa<T, true>(v)
>>
>> I don't think I would know what that means when I see it in code.
>>
>
> Sorry, I was just trying to highlight a different way of doing it, and
> lazily used a bool.  Here's something a little more realistic:
>
> struct NulCheck : std::true_type {};
>
> template <class X, class Check, class Y,
>           typename std::enable_if<Check::value, X>::type * =
nullptr>
> LLVM_NODISCARD inline bool isa(const Y *Val) {
>   return Val && isa<X>(Val);
> }
>
> isa<foo,NulCheck>(v)
>
> Not sure if that's better than isa_and_nonnull or isa_nonnull, but
it's
> shorter than the first, and only slightly longer than the second.
>
>
>
>
>>
>>                        -David
>>
>> >  On Sat, Apr 6, 2019 at 9:45 AM Mehdi AMINI via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>> >
>> >  On Fri, Apr 5, 2019 at 5:15 AM Aaron Ballman via llvm-dev <
>> llvm-dev at lists.llvm.org> wrote:
>> >
>> >  On Thu, Apr 4, 2019 at 12:58 PM Chris Lattner <clattner at
nondot.org>
>> wrote:
>> >  >
>> >  >
>> >  >
>> >  > > On Apr 4, 2019, at 5:37 AM, Don Hinton via llvm-dev
<
>> llvm-dev at lists.llvm.org> wrote:
>> >  > >
>> >  > > I'd like to propose adding `isa_or_null<>` to
replace the
>> following usage pattern that's relatively common in conditionals:
>> >  > >
>> >  > >   var && isa<T>(var)  =>> 
isa_or_null<T>(var)
>> >  > >
>> >  > > And in particular when `var` is a method call which
might be
>> expensive, e.g.:
>> >  > >
>> >  > >   X->foo() && isa<T>(X->foo()) 
=>>  isa_or_null<T>(X->foo())
>> >  > >
>> >  > > The implementation could be a simple wrapper around
isa<>, and
>> while the IR produced is only slightly more efficient, the elimination
of
>> an extra
>> >  call could be worthwhile.
>> >  >
>> >  > I’d love to see this, I agree with downstream comments
though that
>> this name will be confusing.  isa_and_nonnull<>. ?
>> >
>> >  tbh, I don't think the proposed name will be all that
confusing --
>> >
>> >  I am with David on this, this sounds like misleading naming to
me, I
>> would expect true on null value when reading : if
(isa_or_null<T>(var))
>> >
>> >  we're used to _or_null() returning "the right
thing" when given null.
>> >
>> >  I think we're used to have "the right thing"
because the name matches
>> the semantic: the "_or_null()" suffix matches the semantics a
conversion
>> operator
>> >  that returns nullptr on failure.
>> >  It does not translate with isa<> IMO.
>> >
>> >
>> >  isa_and_nonnull<> is a bit of a weird name for me, but I
could
>> >  probably live with it. We could spell it nonnull_and_isa<>
to reflect
>> >  the order of the operations, but that sort of hides the important
part
>> >  of the API (the "isa" bit).
>> >
>> >  isa_nonnulll works fine for me, isa_and_nonnull is a bit verbose
but
>> seems OK as well.
>> >
>> >  For nonnull_and_isa<T>(val) ; it starts to look strangely
close to the
>> pattern !val && isa<T>(val) ; and I'm not sure it is
really such a
>> readability
>> >  improvement anymore?
>> >
>> >  --
>> >  Mehdi
>> >
>> >
>> >
>> >  ~Aaron
>> >
>> >  >
>> >  > -Chris
>> >  >
>> >  _______________________________________________
>> >  LLVM Developers mailing list
>> >  llvm-dev at lists.llvm.org
>> >  https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >
>> >  _______________________________________________
>> >  LLVM Developers mailing list
>> >  llvm-dev at lists.llvm.org
>> >  https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >
>> >  _______________________________________________
>> >  LLVM Developers mailing list
>> >  llvm-dev at lists.llvm.org
>> >  https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>> >
>> > _______________________________________________
>> > LLVM Developers mailing list
>> > llvm-dev at lists.llvm.org
>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190422/766c9e32/attachment.html>

On 22/04/2019 15:15, Don Hinton via llvm-dev wrote:
> Although there were a few no votes, it looks like there's a consensus 
> for adding a `isa_and_nonnull` type operator.  While there were some who 
> preferred `isa_nonnull`, it wasn't overwhelming, and since 
> `isa_and_nonnull` is already committed, I'm going to leave it as 
> `isa_and_nonnull` for the time being.
Maybe I missed something, but it looked to me as if the consensus was 
that `isa_and_some_words<T>(foo)` imposed a higher cognitive load on the 
reader than `foo && isa<T>(foo)`, as well as being more to type in
most
cases, so wasn't worth adding.

David

Hi David:
>From my count, of the 12 people who commented, 8 were in favor of adding
`isa_and_nonnull`, while only 4 were against it.  A few of the 'yes'
votes
also indicated a preference for the shorter `isa_nonnull` alternative --
this was also true of those I asked at the Social last night.  So there may
be a consensus building for a name change as well which could alleviate
some of your cognitive load concerns.

However, the only consensus I can discern from this thread is an agreement
to add `isa_and_nonnull` (or `isa_nonnull`), not that it's not worth it.

Please let me know if I missed something, thanks...
don

On Mon, Apr 29, 2019 at 2:37 AM David Chisnall via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On 22/04/2019 15:15, Don Hinton via llvm-dev wrote:
> > Although there were a few no votes, it looks like there's a
consensus
> > for adding a `isa_and_nonnull` type operator.  While there were some
who
> > preferred `isa_nonnull`, it wasn't overwhelming, and since
> > `isa_and_nonnull` is already committed, I'm going to leave it as
> > `isa_and_nonnull` for the time being.
>
> Maybe I missed something, but it looked to me as if the consensus was
> that `isa_and_some_words<T>(foo)` imposed a higher cognitive load on
the
> reader than `foo && isa<T>(foo)`, as well as being more to
type in most
> cases, so wasn't worth adding.
>
> David
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190503/a204f57b/attachment.html>

On Mon, Apr 29, 2019, 02:37 David Chisnall via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> On 22/04/2019 15:15, Don Hinton via llvm-dev wrote:
> > Although there were a few no votes, it looks like there's a
consensus
> > for adding a `isa_and_nonnull` type operator.  While there were some
who
> > preferred `isa_nonnull`, it wasn't overwhelming, and since
> > `isa_and_nonnull` is already committed, I'm going to leave it as
> > `isa_and_nonnull` for the time being.
>
> Maybe I missed something, but it looked to me as if the consensus was
> that `isa_and_some_words<T>(foo)` imposed a higher cognitive load on
the
> reader than `foo && isa<T>(foo)`, as well as being more to
type in most
> cases, so wasn't worth adding.
>
FWIW, I agree with this and Bogner: this doesn't seem like an improvement
worth the cost.

> David
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190504/6b276102/attachment.html>