Re: [PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.
Either way looks good. If we go with function_ref, we should definitely store a comment why storing function_ref is fine there. Or use a function pointer to make it even clearer that nothing cheesy is going on there. On Thu, May 23, 2019 at 5:28 PM Yitzhak Mandelbaum wrote: > Actually, someone already committed a fix: https://reviews.llvm.org/D62202 > > I can still make this change if it seems worthwhile, but its not strictly > necessary at this point. > > On Thu, May 23, 2019 at 11:14 AM Yitzhak Mandelbaum > wrote: > >> Given that we'll need to store the function reference, is >> llvm::function_ref still the way to go? The comments seem to warn away from >> storing function_refs. >> >> On Thu, May 23, 2019 at 11:06 AM Yitzhak Mandelbaum >> wrote: >> >>> Sounds good. I'll send a fix shortly. Found another bug too (captured a >>> StringRef in a lambda) -- shall i bundle the fixes? >>> >>> On Thu, May 23, 2019 at 9:01 AM Ilya Biryukov >>> wrote: >>> >>>> Maybe go with a runtime parameter (of type llvm::function_ref) instead >>>> of the template parameter? >>>> In any non-trivial example, the runtime costs of another function >>>> pointer should be negligible given that we need to parse the ASTs, run the >>>> matchers, etc. >>>> >>>> On Wed, May 22, 2019 at 10:48 PM Penzin, Petr >>>> wrote: >>>> >>>>> It does not like some part of that instantiation, I am not sure which >>>>> one yet. Let’s see what I can do about it. >>>>> >>>>> >>>>> >>>>> -Petr >>>>> >>>>> >>>>> >>>>> *From:* Yitzhak Mandelbaum [mailto:yitzh...@google.com] >>>>> *Sent:* Wednesday, May 22, 2019 1:37 PM >>>>> *To:* reviews+d61774+public+f458bb6144ae8...@reviews.llvm.org >>>>> *Cc:* Ilya Biryukov ; Penzin, Petr < >>>>> petr.pen...@intel.com>; llvm-comm...@lists.llvm.org; Michał Górny < >>>>> mgo...@gentoo.org>; cfe-commits ; Theko >>>>> Lekena ; Nicolas Lesser ; >>>>> Han Shen >>>>> *Subject:* Re: [PATCH] D61774: [LibTooling] Add RangeSelector library >>>>> for defining source ranges based on bound AST nodes. >>>>> >>>>> >>>>> >>>>> I'm confused by the error given that getStatementsRange is a function >>>>> name. I don't have Visual Studio -- can you find a fix and send a patch? >>>>> I >>>>> wonder if taking the address explicitly is enough? Or, if you know how to >>>>> trigger this error in clang or gcc, I can fix it myself. >>>>> >>>>> >>>>> >>>>> On Wed, May 22, 2019 at 4:31 PM Petr Penzin via Phabricator < >>>>> revi...@reviews.llvm.org> wrote: >>>>> >>>>> penzn added inline comments. >>>>> >>>>> >>>>> >>>>> Comment at: cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp:229 >>>>> +RangeSelector tooling::statements(StringRef ID) { >>>>> + return RelativeSelector(ID); >>>>> +} >>>>> >>>>> Sorry for posting here, haven't gotten my bugzilla access yet >>>>> (requested though). >>>>> >>>>> This breaks with Visual Studio 2017 (15.7.6): >>>>> >>>>> RangeSelector.cpp(229): error C2971: >>>>> '`anonymous-namespace'::RelativeSelector': template parameter 'Func': >>>>> 'getStatementsRange': a variable with non-static storage duration cannot >>>>> be >>>>> used as a non-type argument >>>>> >>>>> >>>>> Repository: >>>>> rL LLVM >>>>> >>>>> CHANGES SINCE LAST ACTION >>>>> https://reviews.llvm.org/D61774/new/ >>>>> >>>>> https://reviews.llvm.org/D61774 >>>>> >>>>> >>>>> >>>> >>>> -- >>>> Regards, >>>> Ilya Biryukov >>>> >>> -- Regards, Ilya Biryukov ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.
Actually, someone already committed a fix: https://reviews.llvm.org/D62202 I can still make this change if it seems worthwhile, but its not strictly necessary at this point. On Thu, May 23, 2019 at 11:14 AM Yitzhak Mandelbaum wrote: > Given that we'll need to store the function reference, is > llvm::function_ref still the way to go? The comments seem to warn away from > storing function_refs. > > On Thu, May 23, 2019 at 11:06 AM Yitzhak Mandelbaum > wrote: > >> Sounds good. I'll send a fix shortly. Found another bug too (captured a >> StringRef in a lambda) -- shall i bundle the fixes? >> >> On Thu, May 23, 2019 at 9:01 AM Ilya Biryukov >> wrote: >> >>> Maybe go with a runtime parameter (of type llvm::function_ref) instead >>> of the template parameter? >>> In any non-trivial example, the runtime costs of another function >>> pointer should be negligible given that we need to parse the ASTs, run the >>> matchers, etc. >>> >>> On Wed, May 22, 2019 at 10:48 PM Penzin, Petr >>> wrote: >>> >>>> It does not like some part of that instantiation, I am not sure which >>>> one yet. Let’s see what I can do about it. >>>> >>>> >>>> >>>> -Petr >>>> >>>> >>>> >>>> *From:* Yitzhak Mandelbaum [mailto:yitzh...@google.com] >>>> *Sent:* Wednesday, May 22, 2019 1:37 PM >>>> *To:* reviews+d61774+public+f458bb6144ae8...@reviews.llvm.org >>>> *Cc:* Ilya Biryukov ; Penzin, Petr < >>>> petr.pen...@intel.com>; llvm-comm...@lists.llvm.org; Michał Górny < >>>> mgo...@gentoo.org>; cfe-commits ; Theko >>>> Lekena ; Nicolas Lesser ; >>>> Han Shen >>>> *Subject:* Re: [PATCH] D61774: [LibTooling] Add RangeSelector library >>>> for defining source ranges based on bound AST nodes. >>>> >>>> >>>> >>>> I'm confused by the error given that getStatementsRange is a function >>>> name. I don't have Visual Studio -- can you find a fix and send a patch? I >>>> wonder if taking the address explicitly is enough? Or, if you know how to >>>> trigger this error in clang or gcc, I can fix it myself. >>>> >>>> >>>> >>>> On Wed, May 22, 2019 at 4:31 PM Petr Penzin via Phabricator < >>>> revi...@reviews.llvm.org> wrote: >>>> >>>> penzn added inline comments. >>>> >>>> >>>> >>>> Comment at: cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp:229 >>>> +RangeSelector tooling::statements(StringRef ID) { >>>> + return RelativeSelector(ID); >>>> +} >>>> >>>> Sorry for posting here, haven't gotten my bugzilla access yet >>>> (requested though). >>>> >>>> This breaks with Visual Studio 2017 (15.7.6): >>>> >>>> RangeSelector.cpp(229): error C2971: >>>> '`anonymous-namespace'::RelativeSelector': template parameter 'Func': >>>> 'getStatementsRange': a variable with non-static storage duration cannot be >>>> used as a non-type argument >>>> >>>> >>>> Repository: >>>> rL LLVM >>>> >>>> CHANGES SINCE LAST ACTION >>>> https://reviews.llvm.org/D61774/new/ >>>> >>>> https://reviews.llvm.org/D61774 >>>> >>>> >>>> >>> >>> -- >>> Regards, >>> Ilya Biryukov >>> >> smime.p7s Description: S/MIME Cryptographic Signature ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.
Given that we'll need to store the function reference, is llvm::function_ref still the way to go? The comments seem to warn away from storing function_refs. On Thu, May 23, 2019 at 11:06 AM Yitzhak Mandelbaum wrote: > Sounds good. I'll send a fix shortly. Found another bug too (captured a > StringRef in a lambda) -- shall i bundle the fixes? > > On Thu, May 23, 2019 at 9:01 AM Ilya Biryukov > wrote: > >> Maybe go with a runtime parameter (of type llvm::function_ref) instead of >> the template parameter? >> In any non-trivial example, the runtime costs of another function pointer >> should be negligible given that we need to parse the ASTs, run the >> matchers, etc. >> >> On Wed, May 22, 2019 at 10:48 PM Penzin, Petr >> wrote: >> >>> It does not like some part of that instantiation, I am not sure which >>> one yet. Let’s see what I can do about it. >>> >>> >>> >>> -Petr >>> >>> >>> >>> *From:* Yitzhak Mandelbaum [mailto:yitzh...@google.com] >>> *Sent:* Wednesday, May 22, 2019 1:37 PM >>> *To:* reviews+d61774+public+f458bb6144ae8...@reviews.llvm.org >>> *Cc:* Ilya Biryukov ; Penzin, Petr < >>> petr.pen...@intel.com>; llvm-comm...@lists.llvm.org; Michał Górny < >>> mgo...@gentoo.org>; cfe-commits ; Theko >>> Lekena ; Nicolas Lesser ; >>> Han Shen >>> *Subject:* Re: [PATCH] D61774: [LibTooling] Add RangeSelector library >>> for defining source ranges based on bound AST nodes. >>> >>> >>> >>> I'm confused by the error given that getStatementsRange is a function >>> name. I don't have Visual Studio -- can you find a fix and send a patch? I >>> wonder if taking the address explicitly is enough? Or, if you know how to >>> trigger this error in clang or gcc, I can fix it myself. >>> >>> >>> >>> On Wed, May 22, 2019 at 4:31 PM Petr Penzin via Phabricator < >>> revi...@reviews.llvm.org> wrote: >>> >>> penzn added inline comments. >>> >>> >>> >>> Comment at: cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp:229 >>> +RangeSelector tooling::statements(StringRef ID) { >>> + return RelativeSelector(ID); >>> +} >>> >>> Sorry for posting here, haven't gotten my bugzilla access yet (requested >>> though). >>> >>> This breaks with Visual Studio 2017 (15.7.6): >>> >>> RangeSelector.cpp(229): error C2971: >>> '`anonymous-namespace'::RelativeSelector': template parameter 'Func': >>> 'getStatementsRange': a variable with non-static storage duration cannot be >>> used as a non-type argument >>> >>> >>> Repository: >>> rL LLVM >>> >>> CHANGES SINCE LAST ACTION >>> https://reviews.llvm.org/D61774/new/ >>> >>> https://reviews.llvm.org/D61774 >>> >>> >>> >> >> -- >> Regards, >> Ilya Biryukov >> > smime.p7s Description: S/MIME Cryptographic Signature ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.
Sounds good. I'll send a fix shortly. Found another bug too (captured a StringRef in a lambda) -- shall i bundle the fixes? On Thu, May 23, 2019 at 9:01 AM Ilya Biryukov wrote: > Maybe go with a runtime parameter (of type llvm::function_ref) instead of > the template parameter? > In any non-trivial example, the runtime costs of another function pointer > should be negligible given that we need to parse the ASTs, run the > matchers, etc. > > On Wed, May 22, 2019 at 10:48 PM Penzin, Petr > wrote: > >> It does not like some part of that instantiation, I am not sure which one >> yet. Let’s see what I can do about it. >> >> >> >> -Petr >> >> >> >> *From:* Yitzhak Mandelbaum [mailto:yitzh...@google.com] >> *Sent:* Wednesday, May 22, 2019 1:37 PM >> *To:* reviews+d61774+public+f458bb6144ae8...@reviews.llvm.org >> *Cc:* Ilya Biryukov ; Penzin, Petr < >> petr.pen...@intel.com>; llvm-comm...@lists.llvm.org; Michał Górny < >> mgo...@gentoo.org>; cfe-commits ; Theko >> Lekena ; Nicolas Lesser ; >> Han Shen >> *Subject:* Re: [PATCH] D61774: [LibTooling] Add RangeSelector library >> for defining source ranges based on bound AST nodes. >> >> >> >> I'm confused by the error given that getStatementsRange is a function >> name. I don't have Visual Studio -- can you find a fix and send a patch? I >> wonder if taking the address explicitly is enough? Or, if you know how to >> trigger this error in clang or gcc, I can fix it myself. >> >> >> >> On Wed, May 22, 2019 at 4:31 PM Petr Penzin via Phabricator < >> revi...@reviews.llvm.org> wrote: >> >> penzn added inline comments. >> >> >> >> Comment at: cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp:229 >> +RangeSelector tooling::statements(StringRef ID) { >> + return RelativeSelector(ID); >> +} >> >> Sorry for posting here, haven't gotten my bugzilla access yet (requested >> though). >> >> This breaks with Visual Studio 2017 (15.7.6): >> >> RangeSelector.cpp(229): error C2971: >> '`anonymous-namespace'::RelativeSelector': template parameter 'Func': >> 'getStatementsRange': a variable with non-static storage duration cannot be >> used as a non-type argument >> >> >> Repository: >> rL LLVM >> >> CHANGES SINCE LAST ACTION >> https://reviews.llvm.org/D61774/new/ >> >> https://reviews.llvm.org/D61774 >> >> >> > > -- > Regards, > Ilya Biryukov > smime.p7s Description: S/MIME Cryptographic Signature ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.
Maybe go with a runtime parameter (of type llvm::function_ref) instead of the template parameter? In any non-trivial example, the runtime costs of another function pointer should be negligible given that we need to parse the ASTs, run the matchers, etc. On Wed, May 22, 2019 at 10:48 PM Penzin, Petr wrote: > It does not like some part of that instantiation, I am not sure which one > yet. Let’s see what I can do about it. > > > > -Petr > > > > *From:* Yitzhak Mandelbaum [mailto:yitzh...@google.com] > *Sent:* Wednesday, May 22, 2019 1:37 PM > *To:* reviews+d61774+public+f458bb6144ae8...@reviews.llvm.org > *Cc:* Ilya Biryukov ; Penzin, Petr < > petr.pen...@intel.com>; llvm-comm...@lists.llvm.org; Michał Górny < > mgo...@gentoo.org>; cfe-commits ; Theko > Lekena ; Nicolas Lesser ; > Han Shen > *Subject:* Re: [PATCH] D61774: [LibTooling] Add RangeSelector library for > defining source ranges based on bound AST nodes. > > > > I'm confused by the error given that getStatementsRange is a function > name. I don't have Visual Studio -- can you find a fix and send a patch? I > wonder if taking the address explicitly is enough? Or, if you know how to > trigger this error in clang or gcc, I can fix it myself. > > > > On Wed, May 22, 2019 at 4:31 PM Petr Penzin via Phabricator < > revi...@reviews.llvm.org> wrote: > > penzn added inline comments. > > > > Comment at: cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp:229 > +RangeSelector tooling::statements(StringRef ID) { > + return RelativeSelector(ID); > +} > > Sorry for posting here, haven't gotten my bugzilla access yet (requested > though). > > This breaks with Visual Studio 2017 (15.7.6): > > RangeSelector.cpp(229): error C2971: > '`anonymous-namespace'::RelativeSelector': template parameter 'Func': > 'getStatementsRange': a variable with non-static storage duration cannot be > used as a non-type argument > > > Repository: > rL LLVM > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D61774/new/ > > https://reviews.llvm.org/D61774 > > > -- Regards, Ilya Biryukov ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
RE: [PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.
It does not like some part of that instantiation, I am not sure which one yet. Let’s see what I can do about it. -Petr From: Yitzhak Mandelbaum [mailto:yitzh...@google.com] Sent: Wednesday, May 22, 2019 1:37 PM To: reviews+d61774+public+f458bb6144ae8...@reviews.llvm.org Cc: Ilya Biryukov ; Penzin, Petr ; llvm-comm...@lists.llvm.org; Michał Górny ; cfe-commits ; Theko Lekena ; Nicolas Lesser ; Han Shen Subject: Re: [PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes. I'm confused by the error given that getStatementsRange is a function name. I don't have Visual Studio -- can you find a fix and send a patch? I wonder if taking the address explicitly is enough? Or, if you know how to trigger this error in clang or gcc, I can fix it myself. On Wed, May 22, 2019 at 4:31 PM Petr Penzin via Phabricator mailto:revi...@reviews.llvm.org>> wrote: penzn added inline comments. Comment at: cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp:229 +RangeSelector tooling::statements(StringRef ID) { + return RelativeSelector(ID); +} Sorry for posting here, haven't gotten my bugzilla access yet (requested though). This breaks with Visual Studio 2017 (15.7.6): RangeSelector.cpp(229): error C2971: '`anonymous-namespace'::RelativeSelector': template parameter 'Func': 'getStatementsRange': a variable with non-static storage duration cannot be used as a non-type argument Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61774/new/ https://reviews.llvm.org/D61774 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.
I'm confused by the error given that getStatementsRange is a function name. I don't have Visual Studio -- can you find a fix and send a patch? I wonder if taking the address explicitly is enough? Or, if you know how to trigger this error in clang or gcc, I can fix it myself. On Wed, May 22, 2019 at 4:31 PM Petr Penzin via Phabricator < revi...@reviews.llvm.org> wrote: > penzn added inline comments. > > > > Comment at: cfe/trunk/lib/Tooling/Refactoring/RangeSelector.cpp:229 > +RangeSelector tooling::statements(StringRef ID) { > + return RelativeSelector(ID); > +} > > Sorry for posting here, haven't gotten my bugzilla access yet (requested > though). > > This breaks with Visual Studio 2017 (15.7.6): > > RangeSelector.cpp(229): error C2971: > '`anonymous-namespace'::RelativeSelector': template parameter 'Func': > 'getStatementsRange': a variable with non-static storage duration cannot be > used as a non-type argument > > > Repository: > rL LLVM > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D61774/new/ > > https://reviews.llvm.org/D61774 > > > > smime.p7s Description: S/MIME Cryptographic Signature ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits