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

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

thanks, left some comments per our offline discussion.



================
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();
+
----------------
VitaNuo wrote:
> 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.
Right. 

As discussed, for extra parameters, we can pass the input parameters via a virtual method call. we could pack all parameters into a struct, we could refine the interface like :

```
class IncludeSpeller {
public:
   struct Input {
      const Header& HeaderToInsert;
      HeaderSearch &HS;
      const FileEntry* MainFile;
   };
   
   virtual std::string spell(const Input&) = 0;
};
using IncludeSpellerRegistry = llvm::Registry<IncludeSpeller>;
std::string spellHeader(const IncludeSpeller::Input& Input);
```


Now for each IncludeSpeller, `Input` provides enough information to implement their strategy.
e.g. for our internal one which needs to resolve all symlinks, we can get the FileManager from the `HeaderSearch`.


================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:96
+    // Spelling physical headers allows for various plug-in strategies.
+    std::string FinalSpelling = MapHeader(H.physical()->tryGetRealPathName());
+    if (!FinalSpelling.empty())
----------------
as discussed, `tryGetRealPathName` doesn't guarantee it returns an absolute file path, and it is not enough to support our internal use case where we need to resolve the symlink (which requires a `FileManager`). 


================
Comment at: clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:1
+//===--- AnalysisTest.cpp -------------------------------------------------===//
+//
----------------
nit: IncludeSpellerTest.cpp


================
Comment at: clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:60
+    RootWithSeparator += llvm::sys::path::get_separator();
+    AbsolutePath.consume_front(llvm::StringRef{RootWithSeparator});
+    return AbsolutePath.str();
----------------
we miss to check the return value here -- if it is false (the `AbsolutePath` doesn't start with the `testRoot()`), then we returns an empty string.

The mental model is that each `IncludeSpeller` subclass only implements their strategy for a particular interesting path pattern, if the absolute path doesn't match the pattern, we should return an empty string, claiming that there is no customization for this `IncludeSpeller`. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150185