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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

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)

> 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143033