[libc++]Enforce `-Wzero-as-null-pointer-constant`

[fsb4000]

Summary

Fixes https://github.com/llvm/llvm-project/issues/43670

From https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0768r1.pdf :

The relational and equality operators for the comparison category types are specified with an
anonymous parameter of unspecified type. This type shall be selected by the implementation such
that these parameters can accept literal 0 as a corresponding argument. [Example: nullptr_t
satisfies this requirement. — end example] In this context, the behavior of a program that supplies
an argument other than a literal 0 is undefined

So after this patch we will accept not literal 0 but consteval zero like

#include <compare>

consteval int test() { return 0;}
auto b = 1 <=> 2 < test();

But before we accepted nullptr. So I guess it's fine and the behavior is undefined in the both cases.

Oh, I see that we rejected nullptr: https://github.com/llvm/llvm-project/blob/93a04a0591c1958d5344f3541157fbd8b8ff7370/libcxx/test/std/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp

That code was accepted. Yes, the probability of it in reality is much lower than auto b = 1 <=> 2 < (1-1). But I think (1-1) is also low enough...

#include <compare>

consteval int test() { return 0;}
auto b = 1 <=> 2 < (int std::_CmpUnspecifiedParam::*)nullptr;

[philnik]

libcxx/include/__compare/ordering.h

45–50

48

This produces way better error messages on most compilers: https://godbolt.org/z/PhjaTav68

libcxx/test/libcxx/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp

1

I think this test should be inside test/libcxx, since it is very implementation-specific.

libcxx/test/std/language.support/cmp/cmp.partialord/partialord.pass.cpp

10 ↗

(On Diff #506349)

Is there a reason we can't enable this globally?

[fsb4000]

removed unneeded _LIBCPP_HIDE_FROM_ABI

added __literal_zero_is_expected function for a better error message.

added -Wzero-as-null-pointer-constant warning for all tests

moved cmp/cmp.categories.pre/zero_type.verify.cpp. test to libcxx tests

Also I created the same PR for MSVC STL: https://github.com/microsoft/STL/pull/3581

And GCC failed with this change: "in ‘constexpr’ expansion of ‘f.test_range()::<lambda()>()’ cc1plus: error: taking address of an immediate function ‘consteval std::__1::_CmpUnspecifiedParam::_CmpUnspecifiedParam(int)" (std/algorithms/alg_sorting/alg_min_max/ranges.min.pass.cpp")

I will try to investigate this.

[fsb4000]

libcxx/include/__compare/ordering.h

45–50

Thanks!

48

Yes, the error messages are better but if I remember correctly we can't use throw because we need to support no-exceptions.

libcxx/test/libcxx/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp

1

Ok.

libcxx/test/std/language.support/cmp/cmp.partialord/partialord.pass.cpp

10 ↗

(On Diff #506349)

Ok, I will try.
Let's see if that break something.

[philnik]

These are a lot more changes then expected. Let's split this up into multiple patches. I would suggest that we first enable the warning inside the clang-tidy test to check our headers and then enable the warning inside the tests. As a third patch we can then change the three way comparison.

[fsb4000]

@philnik , Okay, I'll deal with that tomorrow.

[Mordante]

Thanks for working on this!

In D146372#4204837, @philnik wrote:

These are a lot more changes then expected. Let's split this up into multiple patches. I would suggest that we first enable the warning inside the clang-tidy test to check our headers and then enable the warning inside the tests. As a third patch we can then change the three way comparison.

+1 for splitting the patch especially since the changes fail in the CI.

libcxx/test/std/input.output/iostream.format/input.streams/istream.formatted/istream.formatted.arithmetic/int.pass.cpp

50

Doesn't this work too?

[EricWF]

libcxx/include/__compare/ordering.h

46

Richard Smith and I have talked about creating a special type in Clang to use here.
I think we should investigate this approach more thoroughly before going down the path suggested here.

As is, your change makes the number of values and types accepted as an argument to _CmpUnspecifiedParam MUCH larger.

Before, it would only accept the _LITERAL_ zero, now it accepts anything that evaluates to zero, including (0 + 42 - 42), foo(), etc.

So this makes the library much less conforming than it was. This cannot proceed as is.

Though I grant it's very frustrating that the warning is triggered here. But we'll need Clang's help to resolve that.

libcxx/test/libcxx/language.support/cmp/cmp.categories.pre/zero_type.verify.cpp

22

This change seems wrong. It looks like we're enforcing that an error actually occurs when zero is used.
But those test cases get removed?

Maybe we should disable the warning in this test.