[libcxx-commits] [PATCH] D143033: [libc++] Disable some tests in `math_nodiscard_extensions`.

Nikolas Klauser via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Wed Feb 1 12:17:21 PST 2023


philnik added a comment.

In D143033#4097453 <https://reviews.llvm.org/D143033#4097453>, @var-const wrote:

> In D143033#4095911 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143033



More information about the libcxx-commits mailing list