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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:88
 
+class IncludeSpeller {
+public:
----------------
hokein wrote:
> 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.
> 
> 
> 
Ok, created the test that tests the `spellHeader` API with a dummy speller. I do have to pass the speller object manually, though. If I try to link it via `llvm::Registry`, it creates side effects for other test, e.g., `AnalysisTest.cpp`, etc., which is undesirable.


================
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();
+
----------------
hokein wrote:
> 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.
> 
> Plugins might need extra information, e.g. clangd-configs for remapping quotes to > angles (or full path re-writes)
> Reason to push registry to applications and rather take in a functor in >include_cleaner (or just let it be handled by applications completely?)

This is a quote from our sync notes. I believe the idea was that applications might want to parametrize mapping somehow. When linking via a strategy, you can only provide a class that has a parameterless constructor, though. At least that's my understanding.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150185