[PATCH] D150185: [include-cleaner] Allow multiple strategies for spelling includes.

[Haojian Wu via Phabricator via cfe-commits]

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