Refactor threeway compare operator

[20162026]

reimplement <=> as the previous implementation was questionable and failed in some cases.

based on rules described at: https://en.cppreference.com/w/cpp/container/inplace_vector/operator_cmp

[wusatosi]

Thank you for your contribution! I will review this later!

[JeffGarland]

I'm going to approve this PR -- I'll trust you to make any additional changes as you see fit and merging. Good work actually fixing this since it was just wrong. Welcome @20162026 and thanks for your contribution!

[20162026]

Just noticed that my suggestion will not properly handle unorderable values eg. nan("") == nan("").
I will try to come up with a better solution tomorrow

[JeffGarland]

I think the pre-conditions here mean that if you have nans you're in violation and it's UB

[20162026]

I understand precondition as: T models three_way_comparable OR (< is defined for values of type T and < is a total ordering relationship)

nan (float/double) is three_way_comparable with std::partial_ordering, furthermore std::vector nan compare results in std::partial_ordering::unordered

[wusatosi]

failed in some cases.

Can you list out cases that fails with the original implementation?
Those could be great unit tests!

[20162026]

failed in some cases.

Can you list out cases that fails with the original implementation? Those could be great unit tests!

almost all of the issues I could think of are already covered by tests.
But to name a few, original:

• did size compare before value compare
• had incorrect return type
• would not work with unorderable values
• did not check T preconditions
• did not break the loop after finding the first nonequal element

[wusatosi]

failed in some cases.

Can you list out cases that fails with the original implementation? Those could be great unit tests!

almost all of the issues I could think of are already covered by tests.
But to name a few, original:

• did size compare before value compare
• had incorrect return type
• would not work with unorderable values
• did not check T preconditions
• did not break the loop after finding the first nonequal element

Oh I didn't realize you added test as well!! This is fantastic!

[wusatosi]

Thank you for your contribution. As I understand you still have some progress to make, feel free to @wusatosi to notify me once you're done, I can also approve so you can do that in a separate PR.

Again thank you so much for your contribution!

[20162026]

@wusatosi Im done with the changes

[JeffGarland]

It strikes me that the namespace is backwards here -- should be beman::inplace_vector::detail -- but that's an existing issue of course. And maybe something we don't address clearly in the standard.

[wusatosi]

I might be wrong here, but beman::inplace_vector clashes with the inplace_vector struct under the beman namespace.

[wusatosi]

Good job!