On Fri, Oct 31, 2014 at 2:11 PM, Kaelyn Takata <[email protected]> wrote:
> > > On Fri, Oct 31, 2014 at 1:21 PM, David Blaikie <[email protected]> wrote: > >> >> >> On Fri, Oct 31, 2014 at 12:52 PM, Kaelyn Takata <[email protected]> wrote: >> >>> I believe the function_ref could also be changed to std::function now >>> that it is allowed in LLVM, unless I'm overlooking something? >>> >> >> It could, but that would add memory allocation overhead, etc - >> function_ref is more efficient in the cases where you only need to use it >> within a single full expression (as is the case when the code looks like >> yours "auto Result = TransformTypos(*this, >> std::move(Filter)).Transform(E);" - the TransformTypos object is destroyed >> by the end of the full expression) >> >> I'm not sure what the right answer is here - the code as-is is correct, >> just a little subtle (as Twine is). I wouldn't object to std::function, but >> I don't object to the code as-is either - just mentioning it and wondering >> aloud if there might be a safer (& possibly as efficient) option. >> > > That makes sense. I'd done rvalue references for the function_ref because, > not knowing the details, I was basically using it like std::function (not > passing by value to avoid extra memory overhead, and using rvalue > references instead of regular references so that e.g. lambdas could be > passed directly and a default lambda could be provided for the argument). I > just tried the code with the function_ref passed by value, and everything > seems to work fine; since that seems to be the idiom, I shall switch this > patch to doing so as well. > Welcome to commit with that change - if you want to play around with other ideas to mitigate that lifetime subtlety, that'd be cool (happy to chat/bounce some ideas around, if you like). - David > >> >> - David >> >> >>> >>> On Fri, Oct 31, 2014 at 11:15 AM, David Blaikie <[email protected]> >>> wrote: >>> >>>> Looks like the standard idiom is to pass function_ref by value, rather >>>> than rvalue reference. >>>> >>>> factoring out CorrectDelayedTyposInExpr is the sort of thing you can >>>> commit without pre-commit review (it's not a step backwards in readability, >>>> etc, for the existing code even if it's never reused for more callers - so >>>> it doesn't need to wait for the extra use-cases to justify it) & possibly >>>> leave off the filtering until /that's/ needed. >>>> >>>> (the lifetime semantics of that function_ref will be a bit tricky if a >>>> TransformTypos is ever created as a named variable) >>>> >>>> >>>> >>>> >>>> >>>> >>>> On Wed, Oct 29, 2014 at 12:49 PM, Kaelyn Takata <[email protected]> >>>> wrote: >>>> >>>>> >>>>> One takes an Expr* and the other is a simple wrapper that takes an >>>>> ExprResult instead, and handles checking whether the ExprResult is >>>>> invalid. >>>>> >>>>> Additionally, allow an optional callback that is run on the full result >>>>> of the tree transform, for filtering potential corrections based on the >>>>> characteristics of the resulting expression once all of the typos have >>>>> been replaced. >>>>> --- >>>>> include/clang/Sema/Sema.h | 13 +++++++++++++ >>>>> lib/Sema/SemaExprCXX.cpp | 44 >>>>> ++++++++++++++++++++++++++++---------------- >>>>> 2 files changed, 41 insertions(+), 16 deletions(-) >>>>> >>>>> >>>>> _______________________________________________ >>>>> cfe-commits mailing list >>>>> [email protected] >>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>>> >>>>> >>>> >>> >> >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
