[PATCH] D61774: [LibTooling] Add RangeSelector library for defining source ranges based on bound AST nodes.

[Ilya Biryukov via cfe-commits]

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 <yitzhakm at google.com>
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 <yitzhakm at google.com>
> 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 <yitzhakm at google.com>
>> 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 <ibiryukov at google.com>
>>> 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 <petr.penzin at intel.com>
>>>> 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:yitzhakm at google.com]
>>>>> *Sent:* Wednesday, May 22, 2019 1:37 PM
>>>>> *To:* reviews+D61774+public+f458bb6144ae8e78 at reviews.llvm.org
>>>>> *Cc:* Ilya Biryukov <ibiryukov at google.com>; Penzin, Petr <
>>>>> petr.penzin at intel.com>; llvm-commits at lists.llvm.org; Michał Górny <
>>>>> mgorny at gentoo.org>; cfe-commits <cfe-commits at lists.llvm.org>; Theko
>>>>> Lekena <mlekena at skidmore.edu>; Nicolas Lesser <blitzrakete at gmail.com>;
>>>>> Han Shen <shenhan at google.com>
>>>>> *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 <
>>>>> reviews at 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<CompoundStmt, getStatementsRange>(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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20190523/1189cdff/attachment-0001.html>