nridge accepted this revision. nridge added a comment. This revision is now accepted and ready to land.
Thanks, this is nice low-hanging fruit. ================ Comment at: clang-tools-extra/clangd/Diagnostics.cpp:830 + if (!ReplacementFixes.empty()) { + LastDiag->Fixes.insert(LastDiag->Fixes.end(), ReplacementFixes.begin(), + ReplacementFixes.end()); ---------------- Do we maybe want to assert `Info.getFixItHints().empty()` here, so that if clang later introduces its own fix-it it gets on our radar and we can decide which one to prefer? ================ Comment at: clang-tools-extra/clangd/IncludeFixer.cpp:52 +llvm::Optional<llvm::StringRef> getArgStr(const clang::Diagnostic &Info, + unsigned I) { + switch (Info.getArgKind(I)) { ---------------- nit: Index or Idx? ================ Comment at: clang-tools-extra/clangd/IncludeFixer.cpp:495 + if (auto Edit = Inserter->insert(Name)) + return {Fix{llvm::formatv("Include {0}", Name).str(), {std::move(*Edit)}}}; + return {}; ---------------- In the `note_include_header_or_declare` case, we could pull the second arg from the diagnostic (the name of the symbol) and use it to provide the slightly more detailed hint description that [fixUnresolvedName() does](https://searchfox.org/llvm/rev/168bc7ce7e2ebe6527bf3fdd9262ef5c0deab4fc/clang-tools-extra/clangd/IncludeFixer.cpp#168). Up to you if you think that's worth doing. ================ Comment at: clang-tools-extra/clangd/IncludeFixer.h:43 /// Returns include insertions that can potentially recover the diagnostic. + /// If Info describes a note, it will be replaced by any returned fixes. std::vector<Fix> fix(DiagnosticsEngine::Level DiagLevel, ---------------- The added comment describes the behaviour of calling code, not the behaviour of this method. Perhaps it would make more sense in the calling code? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114667/new/ https://reviews.llvm.org/D114667 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits