[PATCH] D67907: [clangd] Implement a smart version of HeaderSource switch.

[Kadir Cetinkaya via Phabricator via cfe-commits]

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