Don Hinton via llvm-dev
2019-Apr-04 23:37 UTC
[llvm-dev] [RFC] Should we add isa_or_null<>?
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/7ee2ec01/attachment-0001.html>
Craig Topper via llvm-dev
2019-Apr-05 00:10 UTC
[llvm-dev] [RFC] Should we add isa_or_null<>?
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. ~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/ad2b74f6/attachment.html>
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>