[PATCH] D114667: [clangd] Add fixes for clang "include <foo.h>" diagnostics
Nathan Ridge via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Mon Nov 29 00:50:21 PST 2021
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
More information about the cfe-commits
mailing list