[libc++] Make some tests in `math_nodiscard_extensions` Clang-only.

[var-const]

Summary

Clang implicitly adds the [[gnu::const]] attribute to many math
functions from the C standard library functions. Since this behavior is
Clang-specific, make sure the test only runs on Clang.

[philnik]

These are marked [[gnu::const]] through clang though. That's why these are regex matchers, not simple text versions.

[var-const]

In D143033#4095223, @philnik wrote:

These are marked [[gnu::const]] through clang though. That's why these are regex matchers, not simple text versions.

Can you elaborate on how exactly this works? (or perhaps it's documented somewhere)

[philnik]

In D143033#4095228, @var-const wrote:

In D143033#4095223, @philnik wrote:

These are marked [[gnu::const]] through clang though. That's why these are regex matchers, not simple text versions.

Can you elaborate on how exactly this works? (or perhaps it's documented somewhere)

It's not exactly documentation, but you can look at clang/include/clang/Basic/Builtins.def to see what attributes are applied to builtins and what builtins there are.

[var-const]

In D143033#4095259, @philnik wrote:

It's not exactly documentation, but you can look at clang/include/clang/Basic/Builtins.def to see what attributes are applied to builtins and what builtins there are.

Thanks. If I'm reading this correctly, though, this applies to __builtin_foo functions only, right? If that's the case, it's not what I'm concerned about. A platform could define e.g. double fabs(double) that would override our implementation (by design). Clang will create __builtin_fabs and mark it as const, but it won't change the platform's definition of fabs (I presume -- but if it did, it seems there would be no need for us to add the attribute in the first place). But since the platform's fabs overrides our fabs, it doesn't necessarily delegate to __builtin_fabs, and in any case, we don't control its definition.

Taking a step back, I also don't think we should be testing compiler behavior or relying on what is essentially an implementation detail of Clang and presumably GCC.

[philnik]

In D143033#4095618, @var-const wrote:

In D143033#4095259, @philnik wrote:

It's not exactly documentation, but you can look at clang/include/clang/Basic/Builtins.def to see what attributes are applied to builtins and what builtins there are.

Thanks. If I'm reading this correctly, though, this applies to __builtin_foo functions only, right?

No, this also applies to the non-__builtin_ versions. The math functions are declared at about line 1200 (currently).

Taking a step back, I also don't think we should be testing compiler behavior or relying on what is essentially an implementation detail of Clang and presumably GCC.

The [[nodiscard]] declarations are specifically there to match the diagnostics produced by the compiler, so I think we should check that the compiler actually produces the same diagnostic for all overloads. (https://godbolt.org/z/87hMPjeq7)

[var-const]

In D143033#4095911, @philnik wrote:

No, this also applies to the non-__builtin_ versions. The math functions are declared at about line 1200 (currently).

Thanks. It means it applies to our definition of double fabs(double), right? (If that's the case, it doesn't need the nodiscard attribute on Clang, though I guess it's there for the sake of GCC)

The [[nodiscard]] declarations are specifically there to match the diagnostics produced by the compiler, so I think we should check that the compiler actually produces the same diagnostic for all overloads. (https://godbolt.org/z/87hMPjeq7)

In the Godbolt link, the non-double overloads of trunc test our implementation, while trunc(0.) essentially tests that Clang adds the const attribute. I don't think we should be testing Clang behavior.

Moreover, this seems Clang-centric. Does GCC have the same behavior? If not, a platform that compiles with GCC and "overrides" one of the math functions will fail the expectation (though to be fair, we could mark the test as unsupported on GCC).

To be clear, I think the idea of adding the attributes is sound. However, testing it is hard, for reasons outside of our control.

[philnik]

In D143033#4097453, @var-const wrote:

In D143033#4095911, @philnik wrote:

No, this also applies to the non-__builtin_ versions. The math functions are declared at about line 1200 (currently).

Thanks. It means it applies to our definition of double fabs(double), right? (If that's the case, it doesn't need the nodiscard attribute on Clang, though I guess it's there for the sake of GCC)

No. Clang only adds them to overloads with C linkage (https://godbolt.org/z/rjY3xeo3o). GCC also warns with -Wall. I don't know where to see that though. So I guess hypothetically there could be a C library that doesn't have these functions declared extern "C" but I doubt these actually exist.

The [[nodiscard]] declarations are specifically there to match the diagnostics produced by the compiler, so I think we should check that the compiler actually produces the same diagnostic for all overloads. (https://godbolt.org/z/87hMPjeq7)

In the Godbolt link, the non-double overloads of trunc test our implementation, while trunc(0.) essentially tests that Clang adds the const attribute. I don't think we should be testing Clang behavior.

I guess we just disagree here. IMO we should test the Clang implementation if we mirror it, especially if it's pretty much trivial to check.

Moreover, this seems Clang-centric. Does GCC have the same behavior? If not, a platform that compiles with GCC and "overrides" one of the math functions will fail the expectation (though to be fair, we could mark the test as unsupported on GCC).

We can mark it as unsuported on GCC, although it seems kind-of pointless, since GCC will most likely never implement -verify and definitely won't mirror clang warning wording.

To be clear, I think the idea of adding the attributes is sound. However, testing it is hard, for reasons outside of our control.

[ldionne]

I like this change -- depending on the fact that this is a .verify.cpp test to disable this on non-Clang compilers is too subtle IMO.

libcxx/test/libcxx/diagnostics/math_nodiscard_extensions.verify.cpp

39

We should even be able to drop this regex now since this is consistently nodiscard.

164

I think this is what I would do:

// These tests rely on Clang's behaviour of adding [[gnu::const]] to the double overload of most of the functions below.
// Without that attribute being added implicitly, this test can't be checked consistently because its result depends on whether we're
// getting libc++'s own std::foo(double) or the underlying C library's foo(double).
#ifdef TEST_COMPILER_CLANG
std::round(0.); // expected-warning-re {{....}}
std::trunc(0.); // expected-warning-re {{....}}
etc...
#endif

[ldionne]

Further investigation: this test will fail on any platform that compiles with -ffreestanding (which turns off the Clang behaviour to add [[gnu::const]]) where the C Standard library provided std::round doesn't provide the attribute itself. I will have a look at setting that up in the CI.

[var-const]

Address feedback

[var-const]

Address feedback

[philnik]

Why didn't you just mark the whole test as // REQUIRES: clang? It's not like we gain anything from "enabling" the test for other compilers. It just makes it a lot harder to verify that all overloads are checked.

libcxx/test/libcxx/diagnostics/math_nodiscard_extensions.verify.cpp

 //===----------------------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 // UNSUPPORTED: c++03
 
 // We don't control the implementation of the math.h functions on windows
 // UNSUPPORTED: windows
 
-// check that const cmath functions are declared [[nodiscard]]
+// Check that functions from `<cmath>` that Clang marks with the `[[gnu::const]]` attribute are declared
+// `[[nodiscard]]`.
 
 #include <cmath>
+#include "test_macros.h"
 
 void test() {
+  // These tests rely on Clang's behaviour of adding `[[gnu::const]]` to the double overload of most of the functions
+  // below.
+  // Without that attribute being added implicitly, this test can't be checked consistently because its result depends
+  // on whether we're getting libc++'s own `std::foo(double)` or the underlying C library's `foo(double)`.
+#ifdef TEST_COMPILER_CLANG
+  std::ceil(0.);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::fabs(0.);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::floor(0.);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::cbrt(0.);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::copysign(0., 0.);         // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::fmax(0., 0.);             // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::fmin(0., 0.);             // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::nearbyint(0.);            // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::rint(0.);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::round(0.);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+  std::trunc(0.);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
+#endif
+
   std::signbit(0.f);             // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::signbit(0.);              // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::signbit(0.l);             // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::signbit(0);               // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::signbit(0U);              // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::fpclassify(0.f);          // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fpclassify(0.);           // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fpclassify(0.l);          // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
#endif
 
   std::isunordered(0.f, 0.f);    // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::isunordered(0., 0.);      // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::isunordered(0.l, 0.l);    // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::isunordered(0, 0);        // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::isunordered(0U, 0U);      // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::ceil(0.f);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::ceil(0.);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::ceil(0.l);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::ceil(0);                  // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::ceil(0U);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::fabs(0.f);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::fabs(0.);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fabs(0.l);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fabs(0);                  // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fabs(0U);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::floor(0.f);               // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::floor(0.);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::floor(0.l);               // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::floor(0);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::floor(0U);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::cbrt(0.f);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::cbrt(0.);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::cbrt(0.l);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::cbrt(0);                  // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::cbrt(0U);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::copysign(0.f, 0.f);       // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::copysign(0., 0.);         // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::copysign(0.l, 0.l);       // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::copysign(0, 0);           // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::copysign(0U, 0U);         // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::fmax(0.f, 0.f);           // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::fmax(0., 0.);             // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fmax(0.l, 0.l);           // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fmax(0, 0);               // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fmax(0U, 0U);             // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::fmin(0.f, 0.f);           // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::fmin(0., 0.);             // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fmin(0.l, 0.l);           // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fmin(0, 0);               // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::fmin(0U, 0U);             // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::nearbyint(0.f);           // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::nearbyint(0.);            // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::nearbyint(0.l);           // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::nearbyint(0);             // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::nearbyint(0U);            // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::rint(0.f);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::rint(0.);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::rint(0.l);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::rint(0);                  // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::rint(0U);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::round(0.f);               // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::round(0.);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::round(0.l);               // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::round(0);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::round(0U);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 
   std::trunc(0.f);               // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
-  std::trunc(0.);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::trunc(0.l);               // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::trunc(0);                 // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
   std::trunc(0U);                // expected-warning-re {{ignoring return value of function declared with {{.*}} attribute}}
 }