ilya-biryukov added inline comments.
================ Comment at: clangd/Diagnostics.cpp:185 OS << Note.Message; - OS << "\n\n"; - printDiag(OS, Main); + // If there's no structured link between the note and the original diagnostic + // then emit the main diagnostic to give context. ---------------- NIT: the comment looks open to misinterpretation, "no structured link" refers to the fact the clients don't support it, but could be read that we don't know which notes correspond to a main diagnostic. Maybe rephrase to something link "If the client does not support structured links …"? ================ Comment at: clangd/Diagnostics.cpp:280 + Main.relatedInformation->push_back(std::move(RelInfo)); + } } ---------------- NIT: maybe call `OutFn` and return here to avoid checking for `EmitRelatedLocations` again? Would arguably make the code simpler, although would require another call to `OutFn(Main)` outside the if branch. ================ Comment at: clangd/SourceCode.cpp:310 + if (!F) { return None; + } ---------------- NIT: seems unrelated. Maybe revert? ================ Comment at: unittests/clangd/DiagnosticsTests.cpp:62 + return false; + if (toJSON(arg) != toJSON(LSPDiag)) { + *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}", ---------------- Maybe omit the `std::tie()`-based comparison altogether? This would not change the semantics of the matcher, right? ================ Comment at: unittests/clangd/DiagnosticsTests.cpp:259 +#ifdef _WIN32 + "c:\\path\\to\\foo\\bar\\main.cpp", +#else ---------------- maybe use `testPath()` here to avoid PP directives? Repository: rCTE Clang Tools Extra CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60267/new/ https://reviews.llvm.org/D60267 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits