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

[David Blaikie]

On Fri, Oct 31, 2014 at 2:11 PM, Kaelyn Takata <rikka at google.com> wrote:

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

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 <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/02d15002/attachment.html>