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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline 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.
----------------
kadircet wrote:
> 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.
> We will most likely have a few URIs that are being resolved over and over again.
This is true, but having a cache seems to be a premature optimization at the moment. I think the number for the Decl in the main file is relatively small in practice. We could add it when it turns out a performance problem. 


================
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;
----------------
kadircet wrote:
> 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?
good point, I thought the StringMap is sorted by the key value, but it uses hash value. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67907/new/

https://reviews.llvm.org/D67907