aaron.ballman added a comment. In D155809#4527890 <https://reviews.llvm.org/D155809#4527890>, @danlark wrote:
> In D155809#4527847 <https://reviews.llvm.org/D155809#4527847>, @aaron.ballman > wrote: > >> In D155809#4527199 <https://reviews.llvm.org/D155809#4527199>, @danlark >> wrote: >> >>> In D155809#4521494 <https://reviews.llvm.org/D155809#4521494>, @rsmith >>> wrote: >>> >>>> This looks correct to me, but it's still a little subtle. Perhaps it'd be >>>> clearer to map the method to an integer (0 for copy assignment, 1 for move >>>> assignment, 2 for destructor, 3 for equality comparison), and then order >>>> them by that integer? That'd be more obviously a strict weak order. >>> >>> >>> >>> In D155809#4520765 <https://reviews.llvm.org/D155809#4520765>, @shafik >>> wrote: >>> >>>> I am not sure about this change but I think we at least need a test and >>>> this does not seem non-functional if it prevents a crash. >>> >>> This is NFC as it only prevents further assert to fire when stable_sort >>> compares the element with itself >> >> Richard's suggestion makes sense to me as a clarifying change to the code. I >> also agree with Shafik -- if this prevents an assertion from firing in >> practice, then it's a functional change that should come with tests. Or are >> you saying the assertion isn't happening in practice and this is a defensive >> change? > > The assertion happens in debug libcxx mode after > https://reviews.llvm.org/D150264. This is a defensive change, in practice, 2 > same functions cannot happen in this comparator, this is only for preventing > assertions on line 1568 > <https://github.com/llvm/llvm-project/blob/2773098ee3187d5f9daca8938d57657dd89dd36f/clang/lib/AST/VTableBuilder.cpp#L1569> I apologize, but I'm still confused. If this assertion triggers in practice in debug modes with libc++, we should be able to make a stand-alone reproducer that we test as part of these changes within Clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155809/new/ https://reviews.llvm.org/D155809 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
