[PATCH] D103142: [clang][clangd] Use reverse header map lookup in suggestPathToFileForDiagnostics

[Dmitry Polukhin via Phabricator via cfe-commits]

DmitryPolukhin added inline comments.


================
Comment at: clang/lib/Lex/HeaderMap.cpp:265
+  }
+  return ReverseMap.lookup(DestPath);
+}
----------------
bruno wrote:
> How about something along the lines of:
> 
> ```
> ...
> StringRef Key;
> for (unsigned i = 0; i != NumBuckets; ++i) {
>     HMapBucket B = getBucket(i);
>     if (B.Key == HMAP_EmptyBucketKey)
>       continue;
> 
>     Key = getString(B.Key);
>     Optional<StringRef> Prefix = getString(B.Prefix);
>     Optional<StringRef> Suffix = getString(B.Suffix);
>     if (!Key.empty() && Prefix && Suffix) {
>       SmallVector<char, 1024> Buf;
>       Buf.append(Prefix->begin(), Prefix->end());
>       Buf.append(Suffix->begin(), Suffix->end());
>       ReverseMap[StringRef(Buf.begin(), Buf.size())] = Key;
>       break;
>     }
> }
> assert(!Key.empty() && "expected valid key");
> return Key;
> ```
> 
> While proposing this change I've noticed that it would keep looking for other buckets even in face of a valid result, so I've added a `break`. Was that intentional? 
Yes, keep iterating over all key-value pairs in the header map is important to add all elements to the map for subsequent lookups. If we stop on the current match, next lookup will not even try to populate the reverse lookup map (because the map is not empty) but not all elements are in the map due to early return from previous lookup, so subsequent lookup may not find the match even if it exists in the header map. We also cannot expect that DestPath is present in the given header map, so we cannot assert if it is missing. This function should return empty string as an indication that DestPath is not found.

Updated implementation with the review suggestion, please take another look.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103142