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

[Richard Smith]

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

// should be ///.


+  bool trapping = false;

Capital T. Also, MSVC doesn't accept this; please put the initializer in
the constructor.

+  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)?


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

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


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

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/73e414f9/attachment.html>