[PATCH] D67907: [clangd] Implement a smart version of HeaderSource switch.
Haojian Wu via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Sep 27 02:47:57 PDT 2019
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
More information about the cfe-commits
mailing list