r221735 - Create two helpers for running the typo-correction tree transform.
Kaelyn Takata
rikka at google.com
Wed Nov 12 10:37:20 PST 2014
The function_refs are passed around by value without any std::moves as
of r221803.
Thanks for diving into the subtle bug I'd encountered and figuring out just
what was going wrong and how to fix it!
On Tue, Nov 11, 2014 at 5:58 PM, David Blaikie <dblaikie at gmail.com> wrote:
>
>
> On Tue, Nov 11, 2014 at 4:04 PM, Kaelyn Takata <rikka at google.com> wrote:
>
>>
>>
>> On Tue, Nov 11, 2014 at 4:00 PM, David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>>
>>>
>>> On Tue, Nov 11, 2014 at 3:58 PM, Kaelyn Takata <rikka at google.com> wrote:
>>>
>>>>
>>>>
>>>> On Tue, Nov 11, 2014 at 3:38 PM, David Blaikie <dblaikie at gmail.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Tue, Nov 11, 2014 at 3:26 PM, Kaelyn Takata <rikka at google.com>
>>>>> wrote:
>>>>>
>>>>>> Author: rikka
>>>>>> Date: Tue Nov 11 17:26:56 2014
>>>>>> New Revision: 221735
>>>>>>
>>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=221735&view=rev
>>>>>> Log:
>>>>>> Create two helpers for running the typo-correction tree transform.
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Modified:
>>>>>> cfe/trunk/include/clang/Sema/Sema.h
>>>>>> cfe/trunk/lib/Sema/SemaExprCXX.cpp
>>>>>>
>>>>>> Modified: cfe/trunk/include/clang/Sema/Sema.h
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=221735&r1=221734&r2=221735&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/include/clang/Sema/Sema.h (original)
>>>>>> +++ cfe/trunk/include/clang/Sema/Sema.h Tue Nov 11 17:26:56 2014
>>>>>> @@ -2718,6 +2718,18 @@ public:
>>>>>> bool EnteringContext = false,
>>>>>> const ObjCObjectPointerType *OPT =
>>>>>> nullptr);
>>>>>>
>>>>>> + ExprResult
>>>>>> + CorrectDelayedTyposInExpr(Expr *E,
>>>>>> + llvm::function_ref<ExprResult(Expr *)>
>>>>>> Filter =
>>>>>> + [](Expr *E) -> ExprResult { return
>>>>>> E; });
>>>>>>
>>>>>
>>>>> Not sure: is it worth pulling this trivial lambda out into a function
>>>>> & use it for the two sites here? (maybe "ExprResult::makeExprResult(Expr*)"
>>>>> ... though that seems almost silly ("make" functions feel like they should
>>>>> be doing template argument deduction, but that's just because that's their
>>>>> most common use))
>>>>>
>>>>> (if you're keeping it as a lambda, you could write it as "[](Expr *E)
>>>>> { return ExprResult(E); }" if you think that's better
>>>>>
>>>>
>>>> Honestly, I wish I could make the default filter simply be the
>>>> ExprResult constructor, since that's pretty much what it is...
>>>>
>>>
>>> Yeah, if it were any static member/free function you could totally do
>>> this - just not the ctor... pity about that.
>>>
>>>
>>>>
>>>>>
>>>>>> +
>>>>>> + ExprResult
>>>>>> + CorrectDelayedTyposInExpr(ExprResult ER,
>>>>>> + llvm::function_ref<ExprResult(Expr *)>
>>>>>> Filter =
>>>>>> + [](Expr *E) -> ExprResult { return
>>>>>> E; }) {
>>>>>> + return ER.isInvalid() ? ER : CorrectDelayedTyposInExpr(ER.get(),
>>>>>> Filter);
>>>>>> + }
>>>>>> +
>>>>>> void diagnoseTypo(const TypoCorrection &Correction,
>>>>>> const PartialDiagnostic &TypoDiag,
>>>>>> bool ErrorRecovery = true);
>>>>>>
>>>>>> Modified: cfe/trunk/lib/Sema/SemaExprCXX.cpp
>>>>>> URL:
>>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaExprCXX.cpp?rev=221735&r1=221734&r2=221735&view=diff
>>>>>>
>>>>>> ==============================================================================
>>>>>> --- cfe/trunk/lib/Sema/SemaExprCXX.cpp (original)
>>>>>> +++ cfe/trunk/lib/Sema/SemaExprCXX.cpp Tue Nov 11 17:26:56 2014
>>>>>> @@ -5919,6 +5919,7 @@ namespace {
>>>>>> class TransformTypos : public TreeTransform<TransformTypos> {
>>>>>> typedef TreeTransform<TransformTypos> BaseTransform;
>>>>>>
>>>>>> + llvm::function_ref<ExprResult(Expr *)> ExprFilter;
>>>>>> llvm::SmallSetVector<TypoExpr *, 2> TypoExprs;
>>>>>> llvm::SmallDenseMap<TypoExpr *, ExprResult, 2> TransformCache;
>>>>>> llvm::SmallDenseMap<OverloadExpr *, Expr *, 4> OverloadResolution;
>>>>>> @@ -5987,7 +5988,8 @@ class TransformTypos : public TreeTransf
>>>>>> }
>>>>>>
>>>>>> public:
>>>>>> - TransformTypos(Sema &SemaRef) : BaseTransform(SemaRef) {}
>>>>>> + TransformTypos(Sema &SemaRef, llvm::function_ref<ExprResult(Expr
>>>>>> *)> &&Filter)
>>>>>>
>>>>>
>>>>> Pass Filter by value if you're going to copy it anyway?
>>>>>
>>>>
>>>> Bizarre thing, I tried this when I originally updated this patch in
>>>> response to your earlier review feedback and for reasons unknown switching
>>>> this from passing by rvalue-reference to passing by value caused things to
>>>> break... yet just now I tried it (even with my latest in-progress changes)
>>>> and it behaved fine.
>>>>
>>>
>>> \o/... *frysquint*
>>>
>>
>> I need to amend my statement. It isn't the rvalue-ref vs value that
>> breaks things... the magic is actually in the std::move--after sending that
>> email I realized I tested the removal of "&&" but didn't change
>> "ExprFilter(std::move(Filter))" to simply "ExprFilter(Filter)". Once I did
>> that on a client without any other local changed, 7 unit tests had
>> segfaults.
>>
>
> Subtle bug in function_ref is subtle. Fixed in LLVM in r221753 - you
> should now be able to just pass the function_refs around by value, drop the
> std::move - function_ref is so cheap it's silly to explicitly move it
> around.
>
> Richard - let me know how that LLVM change looks (CC'ing you here just so
> you have the extra context to our earlier discussion).
>
>
>>
>>
>>>
>>>>
>>>>>
>>>>>> + : BaseTransform(SemaRef), ExprFilter(std::move(Filter)) {}
>>>>>>
>>>>>> ExprResult RebuildCallExpr(Expr *Callee, SourceLocation LParenLoc,
>>>>>> MultiExprArg Args,
>>>>>> @@ -6012,6 +6014,9 @@ public:
>>>>>> res = TransformExpr(E);
>>>>>> error = Trap.hasErrorOccurred();
>>>>>>
>>>>>> + if (!(error || res.isInvalid()))
>>>>>> + res = ExprFilter(res.get());
>>>>>> +
>>>>>> // Exit if either the transform was valid or if there were no
>>>>>> TypoExprs
>>>>>> // to transform that still have any untried correction
>>>>>> candidates..
>>>>>> if (!(error || res.isInvalid()) ||
>>>>>> @@ -6060,6 +6065,25 @@ public:
>>>>>> };
>>>>>> }
>>>>>>
>>>>>> +ExprResult Sema::CorrectDelayedTyposInExpr(
>>>>>> + Expr *E, llvm::function_ref<ExprResult(Expr *)> Filter) {
>>>>>> + // If the current evaluation context indicates there are
>>>>>> uncorrected typos
>>>>>> + // and the current expression isn't guaranteed to not have typos,
>>>>>> try to
>>>>>> + // resolve any TypoExpr nodes that might be in the expression.
>>>>>> + if (!ExprEvalContexts.empty() && ExprEvalContexts.back().NumTypos
>>>>>> &&
>>>>>> + (E->isTypeDependent() || E->isValueDependent() ||
>>>>>> + E->isInstantiationDependent())) {
>>>>>> + auto TyposResolved = DelayedTypos.size();
>>>>>> + auto Result = TransformTypos(*this,
>>>>>> std::move(Filter)).Transform(E);
>>>>>> + TyposResolved -= DelayedTypos.size();
>>>>>> + if (TyposResolved) {
>>>>>> + ExprEvalContexts.back().NumTypos -= TyposResolved;
>>>>>> + return Result;
>>>>>> + }
>>>>>> + }
>>>>>> + return E;
>>>>>> +}
>>>>>> +
>>>>>> ExprResult Sema::ActOnFinishFullExpr(Expr *FE, SourceLocation CC,
>>>>>> bool DiscardedValue,
>>>>>> bool IsConstexpr,
>>>>>> @@ -6106,21 +6130,9 @@ ExprResult Sema::ActOnFinishFullExpr(Exp
>>>>>> return ExprError();
>>>>>> }
>>>>>>
>>>>>> - // If the current evaluation context indicates there are
>>>>>> uncorrected typos
>>>>>> - // and the current expression isn't guaranteed to not have typos,
>>>>>> try to
>>>>>> - // resolve any TypoExpr nodes that might be in the expression.
>>>>>> - if (ExprEvalContexts.back().NumTypos &&
>>>>>> - (FullExpr.get()->isTypeDependent() ||
>>>>>> - FullExpr.get()->isValueDependent() ||
>>>>>> - FullExpr.get()->isInstantiationDependent())) {
>>>>>> - auto TyposResolved = DelayedTypos.size();
>>>>>> - FullExpr = TransformTypos(*this).Transform(FullExpr.get());
>>>>>> - TyposResolved -= DelayedTypos.size();
>>>>>> - if (TyposResolved)
>>>>>> - ExprEvalContexts.back().NumTypos -= TyposResolved;
>>>>>> - if (FullExpr.isInvalid())
>>>>>> - return ExprError();
>>>>>> - }
>>>>>> + FullExpr = CorrectDelayedTyposInExpr(FullExpr.get());
>>>>>> + if (FullExpr.isInvalid())
>>>>>> + return ExprError();
>>>>>>
>>>>>> CheckCompletedExpr(FullExpr.get(), CC, IsConstexpr);
>>>>>>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> 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/20141112/dba7146e/attachment.html>
More information about the cfe-commits
mailing list