[PATCH] D78715: [clangd] Fix modernize-loop-convert "multiple diag in flight" crash.

Fri Apr 24 02:06:34 PDT 2020

hokein added a comment.

In D78715#1999236 <https://reviews.llvm.org/D78715#1999236>, @sammccall wrote:

> Ah, nice work!
>  Agree it's a hack (and how many more instances are lurking) but I don't know how better to fix it and it's been reported 3 times...

this is the only place in clang-tidy calling `getTypeInfo`.

================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:529
+  // getTypeInfo may emit a diagnostic, we have to call it before constructing
+  // the Diag to avoid the "multiple-diagnostic in flight" crash in clangd.
+  auto TypeWidth = Context->getTypeInfo(Descriptor.ElemType).Width;
----------------
sammccall wrote:
> I'm not sure we need to mention clangd here - I'd expect standalone clang-tidy to be able to hit the same condition?
> At least logically I think the bug exists either way.
yeah logically, but it doesn't trigger the assertion somehow (haven't dig into it). removed it.

 I realized that the code below may change `Descriptor.ElemType`, refined the fix (we store the fixits, and emit the diagnostic at the end of the function).

================
Comment at: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp:275

+TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
+  Annotations Main(R"cpp(
----------------
sammccall wrote:
> Does the bug repro with clang-tidy? If so it'd be nice to make this a clang-tidy test.
unfortunately, not for standalone clang-tidy :(

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78715