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