[PATCH] D130585: [Sema] Return primary merged decl as canonical for concepts

Wed Jul 27 03:10:30 PDT 2022

ilya-biryukov added a comment.

In D130585#3681397 <https://reviews.llvm.org/D130585#3681397>, @ChuanqiXu wrote:

> In D130585#3680189 <https://reviews.llvm.org/D130585#3680189>, @ilya-biryukov wrote:
>
>> I wonder whether the right fix for this should generalize to `Mergeable<T>`.
>> The fact that `Mergeable::getFirstDecl` checks if the decl is coming from the AST file before looking up its primary merged decl is probably a performance optimization, not a deliberate semantic.
>
> Agreed. I think we could omit the `isFromASTFile` part. But if we want to generalize to `Mergeable<T>`, we need to add much more tests. I think we could make it in future patches.

Totally agree, the Mergable change needs to be tested more thoroughly.
I have incorporated the `getCanonicalDecl()` call from your patch here and added test for the corresponding failure to `merge-concepts.cpp` too.
Otherwise my patch did not fix the failure I was encountering in practice (after I double-checked).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130585