hokein added a comment.
Thanks, a few comments to simplify the code/test further.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:94
+/// shortest spelling.
+std::string spellHeader(const IncludeSpellerInput &Input);
} // namespace include_cleaner
----------------
can you move it to the `IncludeSpeller.h` file? I think it is a more suitable
place.
================
Comment at:
clang-tools-extra/include-cleaner/include/clang-include-cleaner/IncludeSpeller.h:22
+/// Provides the necessary information for custom spelling computations.
+struct IncludeSpellerInput {
+ const Header &H;
----------------
This struct is the input for the IncludeSpeller interface, thus I prefer to
move the structure to IncludeSpeller class, and call it `Input`.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:83
-std::string spellHeader(const Header &H, HeaderSearch &HS,
- const FileEntry *Main) {
+std::string spellHeader(const IncludeSpellerInput &Input) {
+ const Header &H = Input.H;
----------------
similar to .h file, I think we should move this implementation to the
`IncludeSpeller.cpp` file.
================
Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:96
+
+ // Fallback to default spelling via header search.
+ bool IsSystem = false;
----------------
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);
}
```
================
Comment at: clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:2
+//===--- IncludeSpellerTest.cpp
+//-------------------------------------------------===//
+//
----------------
line: line 1 and line 2 should be merged into a single line.
================
Comment at:
clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:60
+ AbsolutePath.consume_front(llvm::StringRef{RootWithSeparator});
+ if (!Result)
+ return "";
----------------
nit: inline the Result.
```
if (!AbsolutePath.consume_front())
return "";
return "\"" + AbsolutePath.str() + "\"";
```
================
Comment at:
clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:81
+
+ EXPECT_EQ("foo.h", spellHeader({Header{*FM.getFile(testPath("foo.h"))}, HS,
+ MainFile}));
----------------
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.
================
Comment at:
clang-tools-extra/include-cleaner/unittests/IncludeSpellerTest.cpp:90
+ HS, MainFile}));
+ EXPECT_EQ("\"foo/bar.h\"",
+ spellHeader({Header{*FM.getFile("foo/bar.h")}, HS, MainFile}));
----------------
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.
```
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
