[PATCH] D124287: [modules][ODRHash] Compare ODR hashes to detect mismatches in duplicate ObjCInterfaceDecl.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added a comment.

In D124287#3474369 <https://reviews.llvm.org/D124287#3474369>, @jansvoboda11 wrote:

> This sounds reasonable to me. What use-cases does `ASTStructuralEquivalence` fit better than ODR hashes?

I think the biggest advantage of `ASTStructuralEquivalence` is that it doesn't affect memory consumption by Decls. But it makes repeated comparisons more expensive.

Another `ASTStructuralEquivalence` advantage is that it's easier to add new checks. For example, comparing ivars is pretty trivial with `ASTStructuralEquivalence` but with ODR hash we need to handle that ivar types can be structs/unions, including anonymous and nested structs.

Cannot say that for sure but I think `ASTStructuralEquivalence` is easier to extend and to adopt for different purposes, while ODR hash seems to be more invasive. But this argument is more hand-wavy.

Most likely `ASTStructuralEquivalence` has other positive qualities but that's what I was able to come up off the top of my head. And you can see that in my decision-making `ASTStructuralEquivalence` doesn't look like a compelling option.



================
Comment at: clang/include/clang/Sema/Sema.h:3300
+            typename = std::enable_if_t<std::is_base_of<NamedDecl, T>::value>>
+  bool ActOnDuplicateDefinition(T *Duplicate, T *Previous) {
+    if (Duplicate->getODRHash() != Previous->getODRHash())
----------------
jansvoboda11 wrote:
> I'm not sure I'm a fan of using the exact same function name for checking ODR hashes and structural equivalence.
That's my failure of imagination, will come with something different. And after you've pointed it out, I've realized it's not a good use of overloading too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124287