[PATCH] D155809: [NFC] [Clang] Fix strict weak ordering in ItaniumVTableBuilder

Mon Jul 24 06:07:22 PDT 2023

danlark added a comment.

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>

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155809/new/

https://reviews.llvm.org/D155809