kadircet added a comment.
mostly LG, a few small comments
================
Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:86
+ auto AwardTarget = [&](const char *TargetURI) {
+ if (auto TargetPath = URI::resolve(TargetURI, OriginalFile)) {
+ if (*TargetPath != OriginalFile) // exclude the original file.
----------------
should we have a cache for these uri resolutions?
We will most likely have a few URIs that are being resolved over and over again.
================
Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:114
+ for (auto It = Candidates.begin(); It != Candidates.end(); ++It) {
+ if (It->second > Best->second)
+ Best = It;
----------------
what if there are ties ?
we don't need to do anything clever yet, but we should at least be
deterministic. Currently it depends on stringmap ordering.
Could you pick the candidate that comes first in the lexical order in such
cases?
and add some test cases?
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:141
+ llvm::Optional<std::string> ExpectedSource;
+ } TestCases[] = {{R"cpp(// empty, no header found)cpp", llvm::None},
+ {R"cpp(
----------------
no need for raw string literals?
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:158
+ testPath("b.cpp")}};
+ const std::string &TestFilePath = testPath("TestTU.h");
+ for (const auto &Case : TestCases) {
----------------
maybe just `const std::string` ? testpath is returning a value, not a ref.
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:160
+ for (const auto &Case : TestCases) {
+ TestTU TU = TestTU::withCode(Case.HeaderCode);
+ TU.ExtraArgs.push_back("-xc++-header"); // inform clang this is a header.
----------------
maybe also set `TU.FileName` to `TestFilePath` ?
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:196
+ } TestCases[] = {
+ {R"cpp(// empty, no header found)cpp", llvm::None},
+ {R"cpp(
----------------
again no need for raw literals
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:217
+ };
+ const std::string &TestFilePath = testPath("TestTU.cpp");
+ for (const auto &Case : TestCases) {
----------------
same as above
================
Comment at: clang-tools-extra/clangd/unittests/HeaderSourceSwitchTests.cpp:219
+ for (const auto &Case : TestCases) {
+ TestTU TU = TestTU::withCode(Case.SourceCode);
+ auto AST = TU.build();
----------------
again set `TU.FileName` ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D67907/new/
https://reviews.llvm.org/D67907
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
