kadircet added a comment.
thanks a lot! outline seems good. mostly some comments on implementation
details.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:92
+
+ virtual std::string operator()(llvm::StringRef HeaderPhysicalPath) const = 0;
+};
----------------
we should have some comments explaining the semantics and rationale. maybe
something like:
```
An extension point to let applications introduce custom spelling strategies for
physical headers.
It takes in absolute path to a source file and should return a verbatim include
spelling (with angles/quotes) or an empty string to indicate no customizations
are needed.
```
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:97
+ const FileEntry *Main,
+ const IncludeSpeller *CustomSpeller);
+
----------------
we can change the interface here to just a functor now, e.g.
`llvm::function_ref<std::string(llvm::StringRef /*AbsPath*/)> MapHeader`.
we should also add some comments to explain the contract:
```
Generates a spelling for `H` that can be directly included in `Main`.
When `H` is a physical header, prefers the spelling provided by `MapHeader` if
any, otherwise uses header search info to generate shortest spelling.
```
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:99
+
+typedef llvm::Registry<IncludeSpeller> IncludeSpellingStrategy;
+
----------------
let's move this closer to interface definition.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:101
+
+class ApplyFirstIncludeSpeller : public IncludeSpeller {
+public:
----------------
let's move this into the source file and maybe expose with something like:
```
/// A header mapper that iterates over all registered include spelling
strategies.
/// Note that when there are multiple strategies iteration order is not
specified.
std::function<std::string(llvm::StringRef)> defaultHeaderMapper();
```
you can also make this the default for `mapHeader` parameter in `spellHeader`.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:104
+ ApplyFirstIncludeSpeller() {
+ for (const auto &Strategy :
+ include_cleaner::IncludeSpellingStrategy::entries()) {
----------------
instead of a constructor you can have:
```
static auto *Strategies = []{
auto *Result = new
llvm::SmallVector<std::unique_ptr<include_cleaner::IncludeSpeller>>;
for(auto &Strategy: include_cleaner::IncludeSpellingStrategy::entries()) {
Result->push_back(Strategy.instantiate());
}
}();
```
in the functor implementation.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:64
+ const IncludeSpeller *CustomSpeller) {
+ if (H.kind() == Header::Standard) {
return H.standard().name().str();
----------------
let's keep the switch here, as it'll trigger a warning when there's a
non-matched kind during compiles.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:85
+ bool IsSystem = false;
+ std::string Path = HS.suggestPathToFileForDiagnostics(
+ H.physical(), Main->tryGetRealPathName(), &IsSystem);
----------------
you can use `FinalSpelling` here
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:110
+ Ref.RT == RefType::Explicit) {
+ ApplyFirstIncludeSpeller Speller;
+ Missing.insert(
----------------
we should instantiate this outside the callback instead (to make sure we do it
once). it would become obsolete if you're to use a static variable to store the
strategies though.
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
