[RFC] Should we add isa_or_null<>?

Project Infrastructure LLVM Dev List Archives
#21

Sorry, brain isn’t fully working. I meant to call the function / type or_null instead of not_null

#22

I read isa<T>(or_null(v)) as “v is a T or nullptr”, which does not match the implementation semantics “v is a T and not null”.

#23

In that case my original suggestion of isa(not_null(v)) matches the semantics right?

#24

I have to say not_null(v) reads more like an assertion than a predicate, in which case isa<T>(not_null(v)) reads like it has the exact same semantics that isa<T>(v) has currently—asserts that v is not null.

I don’t dispute that you can make it have the desired semantics, it just won’t look that way.

maybe isaT_or_null<Foo>(v) ? Still looks awkward but maybe less naively misleading.

#25

I have to say `not_null(v)` reads more like an assertion than a predicate, in which case `isa<T>(not_null(v))` reads like it has the exact same semantics that `isa<T>(v)` has currently—asserts that `v` is not null.

I don't dispute that you can *make* it have the desired semantics, it just won't *look* that way.

maybe `isaT_or_null<Foo>(v)` ? Still looks awkward but maybe less naively misleading.

... or we keep using "v && isa<T>(v)" (which is not even longer!), which is unambiguous for everyone.
In the rare special case where the pattern "someExpensiveCall() && isa<T>(someExpensiveCall())" is used,
just assign the result of "someExpensiveCall()" to a variable.

Bruno

#26

Bruno Ricci via llvm-dev <llvm-dev@lists.llvm.org> writes:

#27

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;
}

I like the way you’re thinking, but how about something a bit simpler:

template <class X, bool Check, class Y,
typename std::enable_if<Check, X>::type * = nullptr>
LLVM_NODISCARD inline bool isa(const Y *Val) {
return Val && isa(Val);
}

template <class X, bool Check, class Y,
typename std::enable_if<!Check, X>::type * = nullptr>
LLVM_NODISCARD inline bool isa(const Y *Val) {
return isa(Val);
}

Used like this: isa<T,1>(v) or isa<T, true>(v)

#28

David Greene via llvm-dev <llvm-dev@lists.llvm.org> writes:

Bruno Ricci via llvm-dev <llvm-dev@lists.llvm.org> writes:

I have to say `not_null(v)` reads more like an assertion than a
predicate, in which case `isa<T>(not_null(v))` reads like it has the
exact same semantics that `isa<T>(v)` has currently—asserts that `v`
is not null.

I don't dispute that you can *make* it have the desired semantics,
it just won't *look* that way.

maybe `isaT_or_null<Foo>(v)` ? Still looks awkward but maybe less
naively misleading.

... or we keep using "v && isa<T>(v)" (which is not even longer!),
which is unambiguous for everyone. In the rare special case where
the pattern "someExpensiveCall() && isa<T>(someExpensiveCall())" is
used, just assign the result of "someExpensiveCall()" to a variable.

+1.

+1. Both "if (v && isa<T>(v)) ..." and "if (dyn_cast_or_null<T>(v)) ..."
are easier to understand than any of the suggested isa_foo_bar options.
Why add another way to accomplish something that already has two ways to
do it?

#29

Don Hinton via llvm-dev <llvm-dev@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.

                       -David

#30

Don Hinton via llvm-dev <llvm-dev@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(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.

#31

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

#32

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

#33

Hi David:

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

#34

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.

#35

+1, if we're voting. I don't think it adds to the readability of code
for me personally.

#36

+1 on not adding the new API

#37

Personally, I find having a named predicate clearly expresses intent,
especially for the few cases where we found an expression of the
pattern: foo() && isa<Whatever>(foo()).

~Aaron

#38

Personally, I find having a named predicate clearly expresses intent,
especially for the few cases where we found an expression of the
pattern: foo() && isa(foo()).

This was the original motivation. The clang/llvm code base handles this two step operation in 2 ways, either as Aaron described, or by using dyn_cast_or_null<Whatever>(foo()). So, here are the options as I see them:

  1. foo() && isa<T>(foo()) :
  • pros: correct and easy to understand
  • cons: evaluates foo() twice – this could be an issue if foo() is expensive.
  1. dyn_cast_or_null<T>(foo()) :
  • pros: correct and easy to understand
  • cons: while only evaluating foo() a single time, it does an unnecessary cast.
    has a long name
  1. isa_and_nonnull<T>(foo()) or isa_nonnull<T>(foo()) or something else :
  • pros: correct and easy to understand (perhaps I’m bias)
    evaluated foo() only once
    doesn’t do an unnecessary cast.
  • cons: requires addition of additional operator
    isn’t absolutely needed
    has a long name – but not as long as dyn_cast_or_null<T>()

Personally, I’m not overly invested in any of the above options, though I would need to remove the new operator and modify the clang-tidy check to do the right thing if we do decide to remove it.

However, I did want to mention that the _or_null suffix in cast_or_null<T>() and dyn_cast_or_null<T>() has nothing to do with the return type. The suffix only means that that version of the operator “accepts” null pointers. So, it seems to me that isa_or_null<T>() would have been the best name. Anyone confused by the _or_null suffix should have also had a problem with the cast_or_null<T>() and dyn_cast_or_null<T>() operators for the same reason. It’s possible that might be true, but I wasn’t around and haven’t researched it.

I’ll keep watching this thread and will be happy to take action on whatever you guys decide.

thanks…
don

#39

+1 on not adding the new API as well.