[libcxx-commits] [PATCH] D111738: [libcxx][test] Add tests for std::vector comparisons

[Konstantin Boyarinov via Phabricator via libcxx-commits]

kboyarinov added inline comments.


================
Comment at: libcxx/test/std/containers/sequences/vector/compare.pass.cpp:16
+// bool operator> (const vector& lhs, const vector& rhs);
+// bool operator>=(const vector& lhs, const vector& rhs);
+
----------------
Quuxplusone wrote:
> I'd like someone to confirm that this test is still expected to pass in C++20 after `vector`'s `operator<=>` lands. (@cjdb was working on that a long time ago in D80904.) It would be mildly unfortunate if we had to revisit this test in like a month from now.
I would expect this test to pass when `vector::operator<=>` will be defined, because according to the standard, such an operator should perform per-element comparison using `value_type::operator<=>` if it is well-formed or a set of `value_type::operator<` comparisons. We do not test which operator is called for `value_type` objects and the result of the comparison should not change when `operator<=>` will be defined.


================
Comment at: libcxx/test/support/test_comparisons.h:201
+  }
+};
+
----------------
Quuxplusone wrote:
> In isolation, I'd say "please inline these types into the one test file that uses them." But I'm guessing that you're planning to submit similar followup PRs for `deque`, `list`, etc. etc., which will also use these types?
> Still, these can be simplified:
> - I don't see the benefit of the `int Dummy` parameter in the first two types.
> - I don't see the benefit of `int second` in `LessAndEqComp`.
> - I don't think the cost/benefit ratio is right for the `.fail.cpp` test; I'd eliminate it.
You are right, we already have a set of patches in progress for other containers and these types will be used to test them.

As for the simplification:

> I don't see the benefit of the int Dummy parameter in the first two types.

This parameter is required for negative tests. Consider the following code:

```
struct NoComparisons {};

std::vector<NoComparisons> v1, v2;
(void)(v1 == v2);
(void)(v1 != v2);
```

The compiler will indicate that we have he call `operator==` and `operator !=` for the same instantiation of the vector and according to the fact that `operator!=` reuses `operator==`, only one compilation error will be shown. 

 Dummy template parameter is needed to receive and test separate compilation errors for all comparison operators.

> I don't see the benefit of int second in LessAndEqComp.

Agree - it will be removed

> I don't think the cost/benefit ratio is right for the .fail.cpp test; I'd eliminate it.

I am not sure that I understood this comment correct. Could you please clarify what you mean with cost/benefit ratio here and what should be eliminated.

Thank you in advance.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111738