ilya-biryukov added a comment. An experiment with popping on expression evaluation context failed, I couldn't find a good way to fix the problems described above. Typo correction can and will run after the evaluation context where expression created is popped and the diagnostic we produce depends on the results of typo correction. Emitting diagnostics on some, but not all context pops could be an option, but classifying those seems to be hard, would require looking closely at every expr eval context push.
> Failing that, I can live with this landing as-is, but delaying these > diagnostics to the end of the file is going to result in a bad user > experience whenever it happens -- and I suspect it'll start happening > significantly more than the assert currently fails, because we'll no longer > have this assertion forcing people to call `CorrectDelayedTyposInExpr` when > discarding an expression. In practice, this assertion is not even checked most of the time. As mentioned earlier, it only fires when running `clang -cc1` is triggered explicitly **without** `-disable-free` (and driver passes `-disable-free` by default). We ended up in a situation when this assertion was not firing for awhile; `clangd` and `libclang` crash because of this assertion and nobody else notices because `clang` won't crash and won't produce the errors either. I'm happy to either emit or leave out those errors, but I think we should absolutely get rid of assertion in the Sema destructor that only fires in `clang -cc1` and source-level tools. Placing it at some other place that would help to discover all missing calls to `CorrectTypos` to fix typo correction seems ok. However, this change is aimed at unbreaking `libclang` and `clangd` and not fixing typo correction (it looks like a much harder problem). @rsmith two options for this patch seem to be: - silently ignore the errors (current behavior), - show them to the user at the end of TU (can result in bad UX, but we won't drop any errors on the floor). Which one do you think we should prefer? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64799/new/ https://reviews.llvm.org/D64799 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
