danlark added a comment. In D155809#4550646 <https://reviews.llvm.org/D155809#4550646>, @aaron.ballman wrote:
> 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. This assertion triggers in debug mode for various tests but clang is not tested against libc++ debug mode for now. In non debug mode the assertion is impossible to reach because in practice comp(a, a) is not called for all implementations of sorting in all major standard libraries 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
