[PATCH] D130864: [NFC] Introduce ASTContext::isInSameModule()

[Chuanqi Xu via Phabricator via cfe-commits]

ChuanqiXu planned changes to this revision.
ChuanqiXu marked an inline comment as done.
ChuanqiXu added a comment.

In D130864#3693019 <https://reviews.llvm.org/D130864#3693019>, @ilya-biryukov wrote:

> Thanks for working on this. I have a few major concerns here:
>
> - Do we know that it's a bottleneck in practice? E.g. if the module name have different sizes, comparing strings is actually fast. If the module names are short, we are also in a pretty good situation. Having some benchmarks numbers would help to quantify whether the change is needed.

No, we don't know it is a bottleneck in practice. (Currently there shouldn't be many users for C++20 Modules). The reason to do this is that we (Iain and me) feel bad to compare string frequently. Benchmarks would be good but I am afraid I can't. Since there are not enough existing benchmarks and it looks costful to construct the meaningful benchmarks. Let's not make the perfect to be the enemy of better.

> - `llvm::hash_value` is not a crypto hash, so we should expect collisions from it. OTOH, doing an extra string comparison when hashes match defeats the purpose of this optimization. Collisions are rare, but if someone hits one after the compiler is released there are literally no workarounds.  Instead we can use `llvm::SHA1` or `llvm::SHA256`. (possibly truncated to 128 bits? it's hard to tell how much this is needed without benchmarks.)
> - Why not store the hash directly inside `Module` and compute it on construction after setting name? If we want these comparisons to be fast, we probably want to avoid even the overhead of hash table access.

Good idea. Will try to do.


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

https://reviews.llvm.org/D130864