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