sammccall added a comment. Sorry, last comments were concurrent with the latest snapshot.
Except where noted, the old comments still apply I think. ================ Comment at: clangd/ClangdLSPServer.cpp:317 for (Diagnostic &D : Params.context.diagnostics) { - auto Edits = getFixIts(Params.textDocument.uri.file(), D); - if (!Edits.empty()) { + auto Fixes = getFixIts(Params.textDocument.uri.file(), D); + for (auto &F : Fixes) { ---------------- inline this into the for loop? ================ Comment at: clangd/ClangdLSPServer.h:76 - std::vector<TextEdit> getFixIts(StringRef File, const clangd::Diagnostic &D); + std::vector<Fix> getFixIts(StringRef File, const clangd::Diagnostic &D); ---------------- getFixes? ================ Comment at: clangd/Diagnostics.h:35 + DiagnosticsEngine::Level Severity; + llvm::SmallVector<TextEdit, 1> FixIts; + // Since File is only descriptive, we store a separate flag to distinguish ---------------- sammccall wrote: > sammccall wrote: > > As discussed offline - I think fixits should probably be a different struct > > hanging off the main diagnostic, with a name. Following clang's example > > seems... less than clear here. > > > > They could also be modeled as notes with an optional TextEdit - this seems > > sligthly less clear but more faithful to clang internals*. > > Either way, it should be clear that they're only allowed in *one* of these > > locations - having notes and main diags be distinct types would help. > > > > I think this probably affects how we should expose them through LSP - they > > should be named code actions attached to the original diagnostic. > > Maybe they should also be separate diagnostics, but only if they contribute > > a unique opportunity to the user e.g. they have a non-empty range that > > isn't contained within the diagnostic's range. > > This patch seems like a reasonable place to do that, but also OK if you > > want to defer. > Nit: can we call these "Fix"es, which is a noun (at least in common usage?) > > Clang calls them FixItHint, but I think Fix is no less clear (and shorter) This is done, and looks good to me. ================ Comment at: clangd/Diagnostics.h:40 + +struct PersistentDiag { + /// Main diagnostic. ---------------- sammccall wrote: > I don't understand what "persistent" means in this context. > This does seem to be an is-a relationship rather than has-a, so I'd find > DiagBase/Diag + inheritance clearer. > > But with composition, this one seems rike it should be "Diag" and the type of > Main could be DiagDetails or so? (this happened already, yay!) ================ Comment at: clangd/Diagnostics.h:47 + +/// Represents a note for the diagnostic. Severity of this Diagnostic can only +/// be 'note' or 'remark'. ---------------- nit: severity of notes can... ================ Comment at: clangd/Diagnostics.h:51 + +/// A non-note diagnostic with Notes and fix-its. +struct Diag : DiagBase { ---------------- Seems a bit confusing to use "diagnostic" to mean something importantly different than "diag", and to define top-level diagnostic in terms of notes. Maybe just "A top-level diagnostic that may have Notes and Fixes"? ================ Comment at: clangd/Diagnostics.h:54 + std::vector<Note> Notes; + std::vector<Fix> Fixes; +}; ---------------- I think these deserve some comments: - fixes are *alternative* fixes for this diagnostic, one should be chosen - notes elaborate on the problem, usually pointing to a related piece of code ================ Comment at: clangd/Diagnostics.h:57 + +/// Conventional conversion to LSP diagnostics. Formats the error message of +/// each diagnostic to include all its notes. Notes inside main file are also ---------------- I'm not sure what "conventional" means here? ================ Comment at: unittests/clangd/ClangdUnitTests.cpp:24 + +void PrintTo(const DiagBase &D, std::ostream *O) { + llvm::raw_os_ostream OS(*O); ---------------- can you make these operator<< and move to the main code? This seems a fine general-purpose debug representation (could be tweaked, but also later). That avoids the ODR issue, and makes casual logging easier. ================ Comment at: unittests/clangd/ClangdUnitTests.cpp:70 +class WithFixesMatcher : public testing::MatcherInterface<const Diag &> { +public: ---------------- Ah, and now that you've written this... ;-) For the specific case of "field X matches matcher M" you can use ::testing::Field (you'd keep WithNote, but make it call Field) Messages are good as long as you pass the optional field_name param. ================ Comment at: unittests/clangd/ClangdUnitTests.cpp:149 +/// Matches diagnostic that has exactly one fix with the same range and message +/// as the diagnostic itself. +testing::Matcher<const clangd::Diag &> ---------------- this "same range and message as the diagnostic itself" is an important property of the code being tested, and it's hidden here in a comment of a helper function. I'd probably prefer avoiding this helper altogether (spelling the string twice in the test) so the test behavior/change is obvious when changing the code. Failing that, DiagWithEqualFix or something? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D44142 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits