Sean Silva via llvm-dev
2019-May-05 05:52 UTC
[llvm-dev] [RFC] Should we add isa_or_null<>?
+1 on not adding the new API On Sat, May 4, 2019, 11:51 AM David Blaikie via llvm-dev < llvm-dev at lists.llvm.org> wrote:> +1, if we're voting. I don't think it adds to the readability of code > for me personally. > > On Sat, May 4, 2019 at 11:47 AM Chandler Carruth via llvm-dev > <llvm-dev at lists.llvm.org> wrote: > > > > 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 > > > > _______________________________________________ > > 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/20190504/2310724f/attachment.html>
Aaron Ballman via llvm-dev
2019-May-05 12:46 UTC
[llvm-dev] [RFC] Should we add isa_or_null<>?
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 On Sun, May 5, 2019 at 1:52 AM Sean Silva via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > +1 on not adding the new API > > On Sat, May 4, 2019, 11:51 AM David Blaikie via llvm-dev <llvm-dev at lists.llvm.org> wrote: >> >> +1, if we're voting. I don't think it adds to the readability of code >> for me personally. >> >> On Sat, May 4, 2019 at 11:47 AM Chandler Carruth via llvm-dev >> <llvm-dev at lists.llvm.org> wrote: >> > >> > 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 >> > >> > _______________________________________________ >> > 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
Don Hinton via llvm-dev
2019-May-05 21:51 UTC
[llvm-dev] [RFC] Should we add isa_or_null<>?
On Sun, May 5, 2019 at 5:46 AM Aaron Ballman via llvm-dev < llvm-dev at lists.llvm.org> wrote:> 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()). >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. 2) `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 3) `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> ~Aaron > > On Sun, May 5, 2019 at 1:52 AM Sean Silva via llvm-dev > <llvm-dev at lists.llvm.org> wrote: > > > > +1 on not adding the new API > > > > On Sat, May 4, 2019, 11:51 AM David Blaikie via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> > >> +1, if we're voting. I don't think it adds to the readability of code > >> for me personally. > >> > >> On Sat, May 4, 2019 at 11:47 AM Chandler Carruth via llvm-dev > >> <llvm-dev at lists.llvm.org> wrote: > >> > > >> > 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 > >> > > >> > _______________________________________________ > >> > 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/20190505/e3bc6693/attachment.html>
Quentin Colombet via llvm-dev
2019-May-06 16:04 UTC
[llvm-dev] [RFC] Should we add isa_or_null<>?
+1 on not adding the new API as well.> On May 4, 2019, at 10:52 PM, Sean Silva via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > +1 on not adding the new API > > On Sat, May 4, 2019, 11:51 AM David Blaikie via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: > +1, if we're voting. I don't think it adds to the readability of code > for me personally. > > On Sat, May 4, 2019 at 11:47 AM Chandler Carruth via llvm-dev > <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: > > > > On Mon, Apr 29, 2019, 02:37 David Chisnall via llvm-dev <llvm-dev at lists.llvm.org <mailto: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 <mailto:llvm-dev at lists.llvm.org> > >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> > > > > _______________________________________________ > > LLVM Developers mailing list > > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <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/20190506/a0aaf6fa/attachment.html>