aaron.ballman added a comment. 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. 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
