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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits