[PATCH v2 5/9] Start adding the infrastructure for handling TypoExprs.

[Kaelyn Takata]

On Fri, Jun 27, 2014 at 8:05 AM, Richard Smith <richard at metafoo.co.uk>
wrote:

> +    /// \brief The number of typos encountered during this expression
> evaluation
> +    //context (i.e. the number of TypoExprs created).
>
> // should be ///.
>

*sigh* missed that one. /me wishes vim had better support for doxygen-style
comments.


>
>
> +  bool trapping = false;
>
> Capital T. Also, MSVC doesn't accept this; please put the initializer in
> the constructor.
>

Deleted this entirely; see below.


>
> +  ExprResult TransformLambdaExpr(LambdaExpr *E) { return Owned(E); }
>
> Do you need to do this for blocks and CapturedStmts too?
>
> +    while (!E->Consumer->empty()) {
> +      if (TypoCorrection TC = E->Consumer->getNextCorrection()) {
> [... return ...]
>
> This seems strange to me: rather than having a separate empty() and
> getNextCorrection(), either of which can terminate the search, could you
> instead just have getNextCorrection() (which returns the next correction,
> or an invalid correction if there are no more corrections)?
>

*sigh* you're right--I don't know why I did that since getNextCorrection()
returns an empty correction if there are no more available. Changed the
while+if to simply be "while (TypoCorrection TC =
E->Consumer->getNextCorrection()) {"


>
> +  ExprResult TransformTypoExpr(TypoExpr *E) {
>
> I think, with the way this currently works, if I have 'f(typo1, typo2)'
> then I'll try the first correction for typo1 and typo2, then I'll try the
> second correction for typo1 and typo2, and I'll never try the first
> correction for typo1 + the second correction for typo2. Is that intentional?
>

It wasn't intentional--I already knew of this shortcoming but was having
trouble figuring out how to implement the necessary backtracking. A little
while ago I had a flash of inspiration and realized handling this case
would be fairly easy if I change the TypoCorrectionConsumer to store *all*
of the corrections returned by getNextCorrection (in order) instead of just
the last one and add a method to "reset" the TypoCorrectionConsumer such
that getNextCorrection would re-step through the previously returned
corrections before looking for a new valid correction (i.e. make
getNextCorrection replayable). I shall implement that as a replacement for
patch #4 "Have TypoCorrectionConsumer remember the last TypoCorrection
returned." in v3 of the patch set, though the set does not encounter any
cases in the unit tests that run afoul of the current limitation.


> +  ExprResult TransformExpr(Expr *E) {
> +    if (trapping)
> +      return realTransformExpr(E);
> +    ExprResult res;
> +    trapping = true;
> +    bool error = false;
> +    do {
> +      Sema::SFINAETrap Trap(SemaRef);
> +      res = realTransformExpr(E);
> +      error = Trap.hasErrorOccurred();
> +    } while (error && !TypoExprs.empty());
> +    trapping = false;
> +    for (auto E : TypoExprs) {
> +      TypoExpr *TE = cast<TypoExpr>(E);
> +      if (TE->DiagHandler)
> +        (*TE->DiagHandler)(TE->Consumer->getCurrentCorrection());
> +    }
> +    for (auto E : FailedTypoExprs) {
> +      TypoExpr *TE = cast<TypoExpr>(E);
> +      if (TE->DiagHandler)
> +        (*TE->DiagHandler)(TE->Consumer->getCurrentCorrection());
> +    }
> +    return res;
> +  }
>
> It looks like 'trapping' will only be 'false' here on the outermost
> expression (that is, the one for which ActOnFinishFullExpr calls
> TransformExpr). Can you move this logic to ActOnFinishFullExpr instead? Or
> is this the foundation for something else?
>

Ack, you're right. I've renamed TransformExpr to Transform (I'm open to
better entry-point-ish name suggestions) and realTransformExpr to
TransformExpr since all of the logic in the old TransformExpr is only used
when initially kicking off this TreeTransform. I also deleted 'trapping'
entirely since it's no longer used.


>
> I think we're missing one piece here (though maybe it'll follow later):
> full expressions and expression evaluation contexts don't necessarily exist
> in exact correspondence. I think we need to be more careful to:
>
 - keep track of the TypoExprs within each ExprEvaluationContext
>  - only remove those ones that ActOnFinishFullExpr has processed, and
>  - diagnose any remaining ones when we pop the evaluation context
>

Yeah, I've discovered the lack of correspondence between full expressions
and expression evaluation contexts. When I went back to trying to wire up
CorrectTypoDelayed to DiagnoseEmptyLookup for use by a single caller of
DiagnoseEmptyLookup (specifically, ActOnIdExpression) after mailing out
this patch set, I encountered a number of situations where a TypoExpr was
created and the count in an ExprEvaluationContext was updated but
ActOnFinishFullExpr was never called for the Expr (in addition to the
cases I already mentioned where the typo count has to be propagated up the
stack of expression evaluation contexts). I suspect that handling the
remaining TypoExprs in an evaluation context when the context is popped
will diagnose the TypoExpr too soon in some cases (even when it should have
and otherwise would have been handled in ActOnFinishFullExpr). And unless
I've missed something about the state accessible
from PopExpressionEvaluationContext, finding the statements that contain
the remaining TypoExprs to be able to replace the TypoExprs would be
extremely difficult since Exprs don't contain back-links to their parents.
I'm certainly open to better ideas for tracking when an Expr may have a
TypoExpr within it and needs the tree transform run on it--right now the
typo count in the ExprEvaluationContext is just being used as a flag to
have an idea of an Expr should have the tree transform run on it to deal
with TypoExprs (even with the tree transform being applied from places
besides ActOnFinishFullExpr, such as dealing with the inline initializer
Expr for a class member within the class' declaration).


> On Tue, Jun 17, 2014 at 5:29 PM, Kaelyn Takata <rikka at google.com> wrote:
>
>>
>> The the typo count is propagated up the stack of
>> ExpressionEvaluationContextRecords when one is popped off of to
>> avoid accidentally dropping TypoExprs on the floor. For example,
>> the attempted correction of g() in test/CXX/class/class.mem/p5-0x.cpp
>> happens with an ExpressionEvaluationContextRecord that is popped off
>> the stack prior to ActOnFinishFullExpr being called and the tree
>> transform for TypoExprs being run.
>> ---
>>  include/clang/Sema/Sema.h         |  25 ++++
>>  include/clang/Sema/SemaInternal.h |   5 +-
>>  lib/Sema/SemaExpr.cpp             |   7 +
>>  lib/Sema/SemaExprCXX.cpp          |  77 ++++++++++
>>  lib/Sema/SemaLookup.cpp           | 291
>> ++++++++++++++++++++++++--------------
>>  5 files changed, 299 insertions(+), 106 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/20140627/eea20c58/attachment.html>