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
