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