r221735 - Create two helpers for running the typo-correction tree transform.

David Blaikie dblaikie at gmail.com
Tue Nov 11 17:58:57 PST 2014


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/20141111/3cd1b121/attachment.html>


More information about the cfe-commits mailing list