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.
Details
Details
- Reviewers
philnik ldionne - Group Reviewers
Restricted Project - Commits
- rGd1e066dd8bf0: [libc++] Make some tests in `math_nodiscard_extensions` Clang-only.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
These are marked [[gnu::const]] through clang though. That's why these are regex matchers, not simple text versions.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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)
Comment Actions
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.
Comment Actions
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.
Comment Actions
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 | ||
|---|---|---|
| 20–21 | We should even be able to drop this regex now since this is consistently nodiscard. | |
| 37–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 | |
Comment Actions
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.
Comment Actions
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.
We should even be able to drop this regex now since this is consistently nodiscard.