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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

It looks good overall. I left some comments around the interfaces. Let me know if I miss/misunderstand something.



================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:88
 
+class IncludeSpeller {
+public:
----------------
I think this is an important API (we will create a subclass for our internal use), probably worth a dedicated `IncludeSpeller.h/.cpp` file. 


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:88
 
+class IncludeSpeller {
+public:
----------------
hokein wrote:
> I think this is an important API (we will create a subclass for our internal use), probably worth a dedicated `IncludeSpeller.h/.cpp` file. 
I think it would be nice to have a unittest for it.

You can create a subclass `TestIncludeSpeller` in the unittest, which implements a dummy  include spelling for a particular absolute file path (e.g. a file path starting with `/include-cleaner-test/`), and verify `spellHeader` API return expected results.





================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:96
+  /// or an empty string to indicate no customizations are needed.
+  virtual std::string operator()(llvm::StringRef HeaderPhysicalPath) const = 0;
+};
----------------
nit: maybe name the parameter `AbsolutePath`? I think it is better to mention the absolute file path in the name.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:99
+
+typedef llvm::Registry<IncludeSpeller> IncludeSpellingStrategy;
+
----------------
nit: consider using `using IncludeSpellingStrategy = llvm::Registry<IncludeSpeller>;`, which is a more modernized way. 


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:104
+/// order is not specified.
+std::function<std::string(llvm::StringRef)> defaultHeaderMapper();
+
----------------
It is unclear to me why we need `defaultHeaderMapper` and the parameter `MapHeader` in `spellHeader` in the header.

Do we want the caller of `spellHeader` to provide a different HeaderMapper? I don't see a usecase for that -- the current strategy of is to iterate all extension points, if we find the first available one, we just return it; otherwise we use the default fallback (`suggestPathToFileForDiagnostics`). I believe it is enough for `spellHeader` to cover all our cases.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D150185/new/

https://reviews.llvm.org/D150185