hokein added a comment.
thanks, looks good overall, a few more comments
================
Comment at: clang-tools-extra/clangd/Hover.cpp:1225
- HI.Provider = spellHeader(AST, SM.getFileEntryForID(SM.getMainFileID()), H);
+ HI.Provider = include_cleaner::spellHeader(
+ {H, AST.getPreprocessor().getHeaderSearchInfo(),
----------------
we probably need to add a `IncludeSpeller.h` #include insertion for this file,
and probably for `clangd/IncludeCleaner.cpp` as well
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/IncludeSpeller.h:28
+ const Header &H;
+ HeaderSearch &HS;
+ const FileEntry *Main;
----------------
nit: we can use `const HeaderSearch&` now if you rebase the patch to main
branch.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/IncludeSpeller.h:35
+ /// include spelling (with angles/quotes) or an empty string to indicate no
+ /// customizations are needed.
+ virtual std::string operator()(const Input &Input) const = 0;
----------------
the doc is stale now, please update it accordingly,
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/IncludeSpeller.h:42
+/// strategies, if any. Otherwise, uses header search info to generate
+/// shortest spelling.
+std::string spellHeader(const IncludeSpeller::Input &Input);
----------------
and here as well
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/IncludeSpeller.h:45
+
+using IncludeSpellingStrategy = llvm::Registry<IncludeSpeller>;
+
----------------
nit: can you move this statement immediately right after the `IncludeSpeller`
class?
================
Comment at: clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp:22
+// Fallback strategy to default spelling via header search.
+class DefaultIncludeSpeller : public IncludeSpeller {
+
----------------
nit: can you move this and the `mapHeader` to an anonymous namespace? these
symbols are only used for the `spellHeader` implementation, we should keep it
internal to avoid potential ODR violation.
================
Comment at: clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp:33
+std::string mapHeader(const IncludeSpeller::Input &Input) {
+ auto Spellers = [] {
+ llvm::SmallVector<std::unique_ptr<include_cleaner::IncludeSpeller>> Result;
----------------
we need a `static` key word before the auto -- this is to make sure we only
initiate all spellers once (otherwise, we will create all spellers once per
`spellHeader` call, this can be expensive).
================
Comment at: clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp:59
+ case Header::Physical:
+ llvm::errs() << "spelling physical header\n";
+ // Spelling physical headers allows for various plug-in strategies.
----------------
nit: remove the debugging statement.
================
Comment at: clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp:62
+ std::string FinalSpelling = mapHeader(Input);
+ if (!FinalSpelling.empty())
+ return FinalSpelling;
----------------
no need to check the empty here otherwise we will hit the unreachable statement
below (it is fine to return empty string)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D150185/new/
https://reviews.llvm.org/D150185
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
