[PATCH v2 2/6] Create two helpers for running the typo-correction tree transform.

[Kaelyn Takata]

On Fri, Oct 31, 2014 at 1:21 PM, David Blaikie <dblaikie at gmail.com> wrote:

>
>
> On Fri, Oct 31, 2014 at 12:52 PM, Kaelyn Takata <rikka at google.com> 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 <dblaikie at gmail.com>
>> 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 <rikka at google.com>
>>> 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
>>>> cfe-commits at cs.uiuc.edu
>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>>
>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20141031/c4f59e82/attachment.html>