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

On Thu, Apr 4, 2019 at 10:30 AM David Greene <dag at cray.com> wrote:
> I don't think that's a correct replacement.
>
> if (var && isa<T>(var)) {
>   ...
> }
>
> is not the same as:
>
> if (isa_or_null<T>(var)) {
>   ...
> }
>
> at least according to what "isa_or_null" conveys to me.
>
This is the same convention used by the existing "_or_null" varieties,
i.e., "cast_or_null" and "dyn_cast_or_null".   They accept a
null and
propagate it.  In the "isa" case, it would accept a null and propagate
it
as false.

>
> not_null_and_isa<T> would seem a better fit, or maybe
exists_and_isa<T>.
>
> That said, I'm not sure sure we need a special API for this.  Are
> expensive calls used in the way you describe really common?
>
I've only been looking at the ones involving method calls, but it's not
too
common.  Perhaps a dozen in clang/lib -- haven't run it against the rest of
the code base.

>
>                    -David
>
> Don Hinton via llvm-dev <llvm-dev at lists.llvm.org> writes:
>
> > 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.
> >
> > _______________________________________________
> > 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/20190404/245976fd/attachment.html>

Don Hinton <hintonda at gmail.com> writes:
> >  if (isa_or_null<T>(var)) {
> >    ...
> >  }
> >
> >  at least according to what "isa_or_null" conveys to me.
>
> This is the same convention used by the existing "_or_null"
varieties,
> i.e., "cast_or_null" and "dyn_cast_or_null".  They
accept a null and
> propagate it.  In the "isa" case, it would accept a null and
propagate
> it as false.
isa<> is very different from *cast<>.  *cast<> gives you a
pointer back,
which may be null.  isa<> is precondition check, so it "reads"
differently to me.  If I were to see:

if (isa_or_null<T>(var)) {
  ...
}

I would think, "Ok, the body is fine if var is null."

Instead:

if (exists_and_isa<T>(var)) {
  ...
}

This tells me that the body expects a non-null value.
> >  That said, I'm not sure sure we need a special API for this.  Are
> >  expensive calls used in the way you describe really common?
>
> I've only been looking at the ones involving method calls, but it's
> not too common.  Perhaps a dozen in clang/lib -- haven't run it
> against the rest of the code base.
Thanks for checking.  I don't have a strong opinion about the need
either way, but I do care that the spelling is clear and intuitive.

                           -David

I've added a patch, temporarily using the name Chris suggested.  Please let
me know what you think.

https://reviews.llvm.org/D60291

thanks...
don

On Thu, Apr 4, 2019 at 2:55 PM David Greene <dag at cray.com> wrote:
> Don Hinton <hintonda at gmail.com> writes:
>
> > >  if (isa_or_null<T>(var)) {
> > >    ...
> > >  }
> > >
> > >  at least according to what "isa_or_null" conveys to
me.
> >
> > This is the same convention used by the existing "_or_null"
varieties,
> > i.e., "cast_or_null" and "dyn_cast_or_null".  They
accept a null and
> > propagate it.  In the "isa" case, it would accept a null and
propagate
> > it as false.
>
> isa<> is very different from *cast<>.  *cast<> gives you
a pointer back,
> which may be null.  isa<> is precondition check, so it
"reads"
> differently to me.  If I were to see:
>
> if (isa_or_null<T>(var)) {
>   ...
> }
>
> I would think, "Ok, the body is fine if var is null."
>
> Instead:
>
> if (exists_and_isa<T>(var)) {
>   ...
> }
>
> This tells me that the body expects a non-null value.
>
> > >  That said, I'm not sure sure we need a special API for this.
Are
> > >  expensive calls used in the way you describe really common?
> >
> > I've only been looking at the ones involving method calls, but
it's
> > not too common.  Perhaps a dozen in clang/lib -- haven't run it
> > against the rest of the code base.
>
> Thanks for checking.  I don't have a strong opinion about the need
> either way, but I do care that the spelling is clear and intuitive.
>
>                            -David
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20190404/b831ec94/attachment.html>