aaron.ballman added a comment. In D155809#4550674 <https://reviews.llvm.org/D155809#4550674>, @danlark wrote:
> 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. If it fails in any libc++ CI pipeline, you should be able to craft the same code to fail within Clang's test suite. The changes in the patch all look correct to me; the problem is that the patch claims to be NFC when it's not. It is making a functional change (it fixes assertions you were able to hit) and it has no test coverage for that. 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
