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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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