Don Hinton via llvm-dev
2019-Apr-05 00:17 UTC
[llvm-dev] [RFC] Should we add isa_or_null<>?
On Thu, Apr 4, 2019 at 7:10 PM Craig Topper <craig.topper at gmail.com> wrote:> 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> > ~Craig > > > On Thu, Apr 4, 2019 at 4:37 PM Don Hinton <hintonda at gmail.com> wrote: > >> On Thu, Apr 4, 2019 at 6:29 PM Craig Topper <craig.topper at gmail.com> >> wrote: >> >>> There are a handful of places in LLVM that dosomething like if >>> (dyn_cast_or_null<UndefValue>(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. >> >> >>> >>> ~Craig >>> >>> >>> On Thu, Apr 4, 2019 at 4:16 PM Don Hinton via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>>> 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 >>>>> >>>> _______________________________________________ >>>> 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/bd42749b/attachment.html>
Don Hinton via llvm-dev
2019-Apr-05 01:19 UTC
[llvm-dev] [RFC] Should we add isa_or_null<>?
I'm leaning toward Herbert's suggestion: `nonnull_and_isa`. What do you think? On Thu, Apr 4, 2019 at 7:17 PM Don Hinton <hintonda at gmail.com> wrote:> On Thu, Apr 4, 2019 at 7:10 PM Craig Topper <craig.topper at gmail.com> > wrote: > >> 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 > > >> >> ~Craig >> >> >> On Thu, Apr 4, 2019 at 4:37 PM Don Hinton <hintonda at gmail.com> wrote: >> >>> On Thu, Apr 4, 2019 at 6:29 PM Craig Topper <craig.topper at gmail.com> >>> wrote: >>> >>>> There are a handful of places in LLVM that dosomething like if >>>> (dyn_cast_or_null<UndefValue>(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. >>> >>> >>>> >>>> ~Craig >>>> >>>> >>>> On Thu, Apr 4, 2019 at 4:16 PM Don Hinton via llvm-dev < >>>> llvm-dev at lists.llvm.org> wrote: >>>> >>>>> 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 >>>>>> >>>>> _______________________________________________ >>>>> 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/4cdc2fcb/attachment.html>
Don Hinton via llvm-dev
2019-Apr-05 02:12 UTC
[llvm-dev] [RFC] Should we add isa_or_null<>?
Sorry, meant Hubert. On Thu, Apr 4, 2019 at 8:19 PM Don Hinton <hintonda at gmail.com> wrote:> I'm leaning toward Herbert's suggestion: `nonnull_and_isa`. > > What do you think? > > On Thu, Apr 4, 2019 at 7:17 PM Don Hinton <hintonda at gmail.com> wrote: > >> On Thu, Apr 4, 2019 at 7:10 PM Craig Topper <craig.topper at gmail.com> >> wrote: >> >>> 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 >> >> >>> >>> ~Craig >>> >>> >>> On Thu, Apr 4, 2019 at 4:37 PM Don Hinton <hintonda at gmail.com> wrote: >>> >>>> On Thu, Apr 4, 2019 at 6:29 PM Craig Topper <craig.topper at gmail.com> >>>> wrote: >>>> >>>>> There are a handful of places in LLVM that dosomething like if >>>>> (dyn_cast_or_null<UndefValue>(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. >>>> >>>> >>>>> >>>>> ~Craig >>>>> >>>>> >>>>> On Thu, Apr 4, 2019 at 4:16 PM Don Hinton via llvm-dev < >>>>> llvm-dev at lists.llvm.org> wrote: >>>>> >>>>>> 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 >>>>>>> >>>>>> _______________________________________________ >>>>>> 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/b09f8fec/attachment.html>