Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
thanks a lot! outline seems good. mostly some comments on implementation details.
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h | ||
---|---|---|
112 | 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. | |
117 | 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. | |
119 | let's move this closer to interface definition. | |
121 | 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. | |
124 | 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. | |
clang-tools-extra/include-cleaner/lib/Analysis.cpp | ||
82 | let's keep the switch here, as it'll trigger a warning when there's a non-matched kind during compiles. | |
99 | you can use FinalSpelling here | |
109 | 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. |
It looks good overall. I left some comments around the interfaces. Let me know if I miss/misunderstand something.
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h | ||
---|---|---|
108 | I think this is an important API (we will create a subclass for our internal use), probably worth a dedicated IncludeSpeller.h/.cpp file. | |
108 | 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. | |
116 | nit: maybe name the parameter AbsolutePath? I think it is better to mention the absolute file path in the name. | |
119 | nit: consider using using IncludeSpellingStrategy = llvm::Registry<IncludeSpeller>;, which is a more modernized way. | |
124 | 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. |
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h | ||
---|---|---|
108 | Ok, created the test that tests the spellHeader API with a dummy speller. I do have to pass the speller object manually, though. If I try to link it via llvm::Registry, it creates side effects for other test, e.g., AnalysisTest.cpp, etc., which is undesirable. | |
124 |
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. |
thanks, left some comments per our offline discussion.
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h | ||
---|---|---|
124 | 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. | |
clang-tools-extra/include-cleaner/lib/Analysis.cpp | ||
86 | 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). | |
clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp | ||
1 ↗ | (On Diff #527314) | nit: IncludeSpellerTest.cpp |
60 ↗ | (On Diff #527314) | 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. |
Thanks, a few comments to simplify the code/test further.
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h | ||
---|---|---|
114 | can you move it to the IncludeSpeller.h file? I think it is a more suitable place. | |
clang-tools-extra/include-cleaner/include/clang-include-cleaner/IncludeSpeller.h | ||
22 ↗ | (On Diff #527466) | This struct is the input for the IncludeSpeller interface, thus I prefer to move the structure to IncludeSpeller class, and call it Input. |
clang-tools-extra/include-cleaner/lib/Analysis.cpp | ||
75–76 | similar to .h file, I think we should move this implementation to the IncludeSpeller.cpp file. | |
90 | I think it may be clearer to make this fallback spelling implementation as a DefaultIncludeSpeller class, and append it to the end of Spellers (but not register it to make sure it always the last one of the spelling IncludeSpellingStrategy). The code looks like class DefaultIncludeSpeller : IncludeSpeller { ... }; std::string mapHeader(const IncludeSpellerInput &Input) { static auto Spellers = [] { auto Result = llvm::SmallVector<std::unique_ptr<include_cleaner::IncludeSpeller>>{}; for (const auto &Strategy : include_cleaner::IncludeSpellingStrategy::entries()) Result.push_back(Strategy.instantiate()); Result.push_back(std::make_unique<DefaultIncludeSpeller>()); return Result; }(); .... } std::string spellerHeader(...) { ... case Header::Physical: return mapHeader(Input); } | |
clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp | ||
2 ↗ | (On Diff #527466) | line: line 1 and line 2 should be merged into a single line. |
60 ↗ | (On Diff #527466) | nit: inline the Result. if (!AbsolutePath.consume_front()) return ""; return "\"" + AbsolutePath.str() + "\""; |
81 ↗ | (On Diff #527466) | I think there should be "" around the foo.h, the spellHeader API returns the spelling contains the ""<> characters, so we need to add "" to the result in the DummyIncludeSpeller::operator() method. |
90 ↗ | (On Diff #527466) | I think the unittest is mainly for testing our llvm-registry mechanism work by verifying DummyIncludeSpeller is used for file path starting with clangd-test, and fallback one is used otherwise. we're less interested in the the different file separator per platform. Let's try to simplify the unittest to avoid subtle platform issues, something like below should cover the thing we want: Inputs.ExtraFiles["/include-cleaner-test/foo.h"] = ""; Inputs.ExtraFiles["system_header.h"] = ""; Inputs.ExtraFlags = {"-isystem."}; ... EXPECT_EQ("\"foo.h\"", spellHeader({Header{*FM.getFile("foo.h")}, HS, MainFile})); // spelled by the dummerIncludeSpeller. EXPECT_EQ("<system_header.h>", spellHeader({Header{*FM.getFile("system_header.h")}, HS, MainFile})); // spelled by the fallback one. |
thanks, looks good overall, a few more comments
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1225 ↗ | (On Diff #527872) | we probably need to add a IncludeSpeller.h #include insertion for this file, and probably for clangd/IncludeCleaner.cpp as well |
clang-tools-extra/include-cleaner/include/clang-include-cleaner/IncludeSpeller.h | ||
28 ↗ | (On Diff #527872) | nit: we can use const HeaderSearch& now if you rebase the patch to main branch. |
35 ↗ | (On Diff #527872) | the doc is stale now, please update it accordingly, |
42 ↗ | (On Diff #527872) | and here as well |
45 ↗ | (On Diff #527872) | nit: can you move this statement immediately right after the IncludeSpeller class? |
clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp | ||
22 ↗ | (On Diff #527872) | nit: can you move this and the mapHeader to an anonymous namespace? these symbols are only used for the spellHeader implementation, we should keep it internal to avoid potential ODR violation. |
33 ↗ | (On Diff #527872) | we need a static key word before the auto -- this is to make sure we only initiate all spellers once (otherwise, we will create all spellers once per spellHeader call, this can be expensive). |
59 ↗ | (On Diff #527872) | nit: remove the debugging statement. |
62 ↗ | (On Diff #527872) | no need to check the empty here otherwise we will hit the unreachable statement below (it is fine to return empty string) |
Thanks for the comments!
clang-tools-extra/clangd/Hover.cpp | ||
---|---|---|
1225 ↗ | (On Diff #527872) | Yeah also to the IncludeCleanerCheck.cpp now that I've rebased to main. |
Thanks, looks great!
clang-tools-extra/include-cleaner/include/clang-include-cleaner/IncludeSpeller.h | ||
---|---|---|
22 ↗ | (On Diff #528315) | would be nice to add some documentation for this interface, something like IncludeSpeller provides an extension point to allow clients implement custom include spelling strategies applications for physical headers. |
23 ↗ | (On Diff #528315) | nit: remove the extra blank line. |
clang-tools-extra/include-cleaner/lib/IncludeSpeller.cpp | ||
24 ↗ | (On Diff #528315) | add a public:. |
33 ↗ | (On Diff #528315) | nit: maybe name it spellPhysicalHeader( I'd avoid using map, it reminds me too much on the stdlib mapping). |
I think this is an important API (we will create a subclass for our internal use), probably worth a dedicated IncludeSpeller.h/.cpp file.