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

[Kaelyn Takata]

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

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

>
>
>> +      : 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/66f6d305/attachment.html>