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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
