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. > > > - 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
