[RFC] Should we add isa_or_null<>?

Project Infrastructure LLVM Dev List Archives
1

I’d like to propose adding isa_or_null<> to replace the following usage pattern that’s relatively common in conditionals:

var && isa(var) =>> isa_or_null(var)

And in particular when var is a method call which might be expensive, e.g.:

X->foo() && isa(X->foo()) =>> isa_or_null(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.

2

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.

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?

                   -David

Don Hinton via llvm-dev <llvm-dev@lists.llvm.org> writes:

3

I don’t think that’s a correct replacement.

if (var && isa(var)) {
…
}

is not the same as:

if (isa_or_null(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 would seem a better fit, or maybe exists_and_isa.

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.

4

I’d love to see this, I agree with downstream comments though that this name will be confusing. isa_and_nonnull<>. ?

-Chris

5

>
> 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 --
we're used to _or_null() returning "the right thing" when given null.
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).

~Aaron

6

Don Hinton <hintonda@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

7

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

8

There are a handful of places in LLVM that dosomething like if (dyn_cast_or_null(P->hasConstantValue()))

9

There are a handful of places in LLVM that dosomething like if (dyn_cast_or_null(P->hasConstantValue()))

Yes, I’ve seen those, but while working on a new checker, I was advised that replacing X && isa<Y>(X) with dyn_cast_or_null<Y>(X) was suboptimal, and it was suggested something like a isa_or_null style operator would better express what was actually going on, i.e., we are expecting a bool, not a pointer.

10

Agreed that the new isa_or_null style is better. Just wanted mention the other style so we know we should migrate those to the new one.

11

Agreed that the new isa_or_null style is better. Just wanted mention the other style so we know we should migrate those to the new one.

I have a checker under review that could be enhanced to do that – though it currently replaces X->foo() && isa<Y>(X->foo()) with dyn_cast_or_null<Y>(X->foo()).

Please see: https://reviews.llvm.org/D59802

thanks…
don

12

I’m leaning toward Herbert’s suggestion: nonnull_and_isa.

What do you think?

13

Sorry, meant Hubert.

14

I’d like to propose adding isa_or_null<> to replace the following usage pattern that’s relatively common in conditionals:

var && isa(var) =>> isa_or_null(var)

And in particular when var is a method call which might be expensive, e.g.:

X->foo() && isa(X->foo()) =>> isa_or_null(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 –
we’re used to _or_null() returning “the right thing” when given null.
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).

I think “isa_nonnull” would read fine too. To me, the extra “and” makes the ordering more of an issue.

– HT

15

+1 for “isa_nonnull”

–paulr

16

At the risk of bikeshedding:

To me, isa_nonnull sounds as if the caller is guaranteeing that the argument is nonnull. I don't think I've seen it in LLVM, but elsewhere I've come across a convention of adding nonnull variants of functions that skip null checks and pass the non-null restriction to the caller.

I wonder if the better solution is to rename isa to isa_nonnull and introduce a new isa that can take a null argument. If these have the correct nullability annotations then anyone building with clang should get a warning if they use the wrong one...

David

17

Hi David:

I think “isa_nonnull” would read fine too. To me, the extra “and” makes
the ordering more of an issue.

At the risk of bikeshedding:

To me, isa_nonnull sounds as if the caller is guaranteeing that the
argument is nonnull. I don’t think I’ve seen it in LLVM, but elsewhere
I’ve come across a convention of adding nonnull variants of functions
that skip null checks and pass the non-null restriction to the caller.

I wonder if the better solution is to rename isa to isa_nonnull and
introduce a new isa that can take a null argument. If these have the
correct nullability annotations then anyone building with clang should
get a warning if they use the wrong one…

I like your logic, but not sure it’s feasible to detect which one you should use in all cases, e.g., if the user already knows p can’t be null, they just use isa. However, if it could be null, they use p && isa<X>(p), cast_or_null or dyn_cast_or_null. So, we could switch all the current uses of isa to the null variety when we make the change, but wouldn’t we have to trust the user from them on.

Also, wouldn’t this be a pretty big change across llvm and all subprojects, and be an issue for downstream projects.

thanks…
don

18

It would involve a lot of code churn for LLVM (so may not be a good idea for that reason), but for downstream projects it wouldn’t be a breaking change - if they use the null-safe version then their code is still correct, just slightly suboptimal (though I’d expect the compiler to be able to elide the null check in a lot of places).

David

19

I’d like to propose adding isa_or_null<> to replace the following usage pattern that’s relatively common in conditionals:

var && isa(var) =>> isa_or_null(var)

And in particular when var is a method call which might be expensive, e.g.:

X->foo() && isa(X->foo()) =>> isa_or_null(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(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(val) ; it starts to look strangely close to the pattern !val && isa(val) ; and I’m not sure it is really such a readability improvement anymore?

20

What about a type not_null_impl and we could write:

then you could just write bool x = isa(not_null(val));

We provide a function not_null that returns a not_null_impl:

template
not_null_impl not_null(T *t) { return not_null_impl{t}; }

and a specialization of isa that takes a not_null_impl

template<typename T, typename U>
isa<T, not_null_impl>(const not_null_impl &u) {

return u ? isa(*u) : false;
}