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