danlark added a comment. In D155809#4550663 <https://reviews.llvm.org/D155809#4550663>, @aaron.ballman wrote:
> In D155809#4550654 <https://reviews.llvm.org/D155809#4550654>, @danlark wrote: > >> 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 > > Okay, I think you should take the existing tests that trigger the assertion > and reduce the code down to just what's needed to trigger the assertion, then > add that code as a test case to Clang so that we can demonstrate the assert > happens before your patch and doesn't happen after your patch. We've got a > special lit mode (`// REQUIRES: asserts` as a comment near the `RUN` line) to > enable the test only in asserts builds so you don't have to worry about > assertions disabled changing the test behavior. libc++ debug mode is different from assertions in LLVM main library (first is controlled with -D_LIBCPP_ENABLE_DEBUG_MODE, second is with -UNDEBUG). I claim that now I cannot write the test which fails in any existing CI configuration. And I cannot add a new version of CI because of failing tests. That's why I defensively clean up comparators to enable CI in more modes. I made the change as easy as possible to prove that it does not harm the sorting overall. 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
