[PATCH] D62648: [Sema][Typo] Fix assertion failure for expressions with multiple typos

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

Thanks, this looks good; just some nits.



================
Comment at: lib/Sema/SemaExprCXX.cpp:7616
   /// anything (having been passed an empty TypoCorrection).
-  void EmitAllDiagnostics() {
+  void EmitAllDiagnostics(bool isAmbiguous) {
     for (TypoExpr *TE : TypoExprs) {
----------------
Please capitalize this parameter name to match the surrounding code style.


================
Comment at: lib/Sema/SemaExprCXX.cpp:7620-7621
       if (State.DiagHandler) {
-        TypoCorrection TC = State.Consumer->getCurrentCorrection();
-        ExprResult Replacement = TransformCache[TE];
+        TypoCorrection TC = isAmbiguous ? TypoCorrection() : State.Consumer->getCurrentCorrection();
+        ExprResult Replacement = isAmbiguous ? ExprError() : TransformCache[TE];
 
----------------
Reflow to 80 columns.


================
Comment at: lib/Sema/SemaExprCXX.cpp:7686
+  // only succeed if it is able to correct all typos in the given expression.
+  ExprResult CheckForRecursiveTypos(ExprResult Res, bool &outIsAmbiguous) {
+    if (Res.isInvalid()) {
----------------
`outIsAmbiguous` -> `IsAmbiguous` (we don't use an `out` prefix for output parameters).


================
Comment at: lib/Sema/SemaExprCXX.cpp:7694-7695
+
+    auto SavedTypoExprs = TypoExprs;
+    auto SavedAmbiguousTypoExprs = AmbiguousTypoExprs;
+    llvm::SmallSetVector<TypoExpr *, 2>
----------------
Use `std::move` here.


================
Comment at: lib/Sema/SemaExprCXX.cpp:7696-7699
+    llvm::SmallSetVector<TypoExpr *, 2>
+        RecursiveTypoExprs, RecursiveAmbiguousTypoExprs;
+    TypoExprs = RecursiveTypoExprs;
+    AmbiguousTypoExprs = RecursiveAmbiguousTypoExprs;
----------------
Call `TypoExprs.clear()` rather than copying from a default-constructed object.


================
Comment at: lib/Sema/SemaExprCXX.cpp:7723-7724
+
+    TypoExprs = SavedTypoExprs;
+    AmbiguousTypoExprs = SavedAmbiguousTypoExprs;
+
----------------
And `std::move` here too.


================
Comment at: lib/Sema/SemaExprCXX.cpp:7778-7779
+          }
+        } while((Next = State.Consumer->peekNextCorrection()) &&
+            Next.getEditDistance(false) == TC.getEditDistance(false));
+
----------------
Apply clang-format here. (Or: add space after `while` on the first line, and indent the second line so it begins in the same column as `(Next =[...]`).


================
Comment at: lib/Sema/SemaExprCXX.cpp:7781-7785
+        if (!outIsAmbiguous) {
+          AmbiguousTypoExprs.remove(TE);
+          State.Consumer->restoreSavedPosition();
+        } else
+          break;
----------------
Reverse the condition of this `if` so you can early-exit from the loop and remove the `else`.


================
Comment at: lib/Sema/SemaExprCXX.cpp:7819
   ExprResult Transform(Expr *E) {
-    ExprResult Res;
-    while (true) {
-      Res = TryTransform(E);
-
-      // Exit if either the transform was valid or if there were no TypoExprs
-      // to transform that still have any untried correction candidates..
-      if (!Res.isInvalid() ||
-          !CheckAndAdvanceTypoExprCorrectionStreams())
-        break;
-    }
-
-    // Ensure none of the TypoExprs have multiple typo correction candidates
-    // with the same edit length that pass all the checks and filters.
-    // TODO: Properly handle various permutations of possible corrections when
-    // there is more than one potentially ambiguous typo correction.
-    // Also, disable typo correction while attempting the transform when
-    // handling potentially ambiguous typo corrections as any new TypoExprs will
-    // have been introduced by the application of one of the correction
-    // candidates and add little to no value if corrected.
-    SemaRef.DisableTypoCorrection = true;
-    while (!AmbiguousTypoExprs.empty()) {
-      auto TE  = AmbiguousTypoExprs.back();
-      auto Cached = TransformCache[TE];
-      auto &State = SemaRef.getTypoExprState(TE);
-      State.Consumer->saveCurrentPosition();
-      TransformCache.erase(TE);
-      if (!TryTransform(E).isInvalid()) {
-        State.Consumer->resetCorrectionStream();
-        TransformCache.erase(TE);
-        Res = ExprError();
-        break;
-      }
-      AmbiguousTypoExprs.remove(TE);
-      State.Consumer->restoreSavedPosition();
-      TransformCache[TE] = Cached;
-    }
-    SemaRef.DisableTypoCorrection = false;
+    bool isAmbiguous = false;
+    ExprResult Res = RecursiveTransformLoop(E, isAmbiguous);
----------------
`isAmbiguous` -> `IsAmbiguous`.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62648/new/

https://reviews.llvm.org/D62648