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 <yitzh...@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 <yitzh...@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 <yitzh...@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 <ibiryu...@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.pen...@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:yitzh...@google.com] >>>>> *Sent:* Wednesday, May 22, 2019 1:37 PM >>>>> *To:* reviews+d61774+public+f458bb6144ae8...@reviews.llvm.org >>>>> *Cc:* Ilya Biryukov <ibiryu...@google.com>; Penzin, Petr < >>>>> petr.pen...@intel.com>; llvm-comm...@lists.llvm.org; Michał Górny < >>>>> mgo...@gentoo.org>; cfe-commits <cfe-commits@lists.llvm.org>; Theko >>>>> Lekena <mlek...@skidmore.edu>; Nicolas Lesser <blitzrak...@gmail.com>; >>>>> Han Shen <shen...@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 < >>>>> 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<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
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
