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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
