[libcxx-commits] [PATCH] D116878: [libcxx][test] Add missing includes and suppress warnings
Arthur O'Dwyer via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Sun Jan 9 03:09:44 PST 2022
Quuxplusone added subscribers: EricWF, Mordante, Quuxplusone.
Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.
LGTM % comments; thanks for the cleanup!
Personally I'd like to see (or create) followups that
- eliminate `TEST_IGNORE_NODISCARD` in favor of `(void)`
- refactor `concept.swappable/swappable.pass.cpp`
================
Comment at: libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable.pass.cpp:153-154
throwable_adl_swappable y[] = {{4}, {5}, {6}, {7}};
- return check_swap_22(x, y) && !noexcept(std::ranges::swap(x, y));
+ constexpr auto not_noexcept = !noexcept(std::ranges::swap(x, y));
+ return check_swap_22(x, y) && not_noexcept;
}
----------------
Looks like this should be
```
constexpr bool check_throwable_adl_swappable_arrays() {
throwable_adl_swappable x[] = {{0}, {1}, {2}, {3}};
throwable_adl_swappable y[] = {{4}, {5}, {6}, {7}};
ASSERT_NOT_NOEXCEPT(std::ranges::swap(x, y));
assert(check_swap_22(x, y));
return true;
}
static_assert(check_throwable_adl_swappable_arrays());
```
except then there's a lot more cleanup that can be done in here.
I'm guessing you don't care to tackle //all// that cleanup?...
================
Comment at: libcxx/test/std/containers/sequences/vector/access.pass.cpp:51
try {
- c.at(n);
+ TEST_IGNORE_NODISCARD c.at(n);
assert(false);
----------------
Oh that's interesting. Looks like this macro was added in D40065, at @EricWF's request; but I've never noticed it before. In April 2021 we (I) //removed// a similar macro, `_LIBCPP_UNUSED_VAR(x)`, in D100737. There are also places where we do `(void)(x == y);` to suppress unused-result warnings that are unrelated to `[[nodiscard]]`.
I think we should eliminate `TEST_IGNORE_NODISCARD` in favor of `(void)`, in a separate commit.
(No action required in this PR though.)
================
Comment at: libcxx/test/std/containers/views/span.cons/initializer_list.pass.cpp:24-31
constexpr int count(std::span<const Sink> sp) {
- return sp.size();
+ return static_cast<int>(sp.size());
}
template<int N>
constexpr int countn(std::span<const Sink, N> sp) {
+ return static_cast<int>(sp.size());
----------------
Would changing the return type to `size_t` silence MSVC's complaint? (Or just move it further down to where we're comparing against a signed int `10`?)
================
Comment at: libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp:32
constexpr double *insert(double *pos, int value) {
*pos = value;
return pos;
----------------
Would `*pos = double(value);` or `*pos = static_cast<double>(value);` silence the MSVC warning? If so, I think that'd be better than `#pragma warning`.
================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:44-48
}
assert(false);
+#ifdef _MSC_VER
+ __assume(false); // *sigh* `assert` can return on MSVC - swear this one won't
+#endif
----------------
================
Comment at: libcxx/test/std/numerics/bit/byteswap.pass.cpp:77
test_num<bool>(false, false);
- test_num<char>(0xCD, 0xCD);
+ test_num<char>(static_cast<char>(0xCD), static_cast<char>(0xCD));
test_num<unsigned char>(0xEF, 0xEF);
----------------
Here and above, can we get away with `char(0xCD)`, `int16_t(0xCDEF)`, `int8_t(0xAB)`? Those would be easier on the eyes.
================
Comment at: libcxx/test/std/ranges/range.factories/range.iota.view/size.pass.cpp:67-68
{
const std::ranges::iota_view<int, int> io(0, std::numeric_limits<int>::max());
- assert(io.size() == std::numeric_limits<int>::max());
+ assert(io.size() == static_cast<unsigned int>(std::numeric_limits<int>::max()));
}
----------------
Are we saying that
```
ASSERT_SAME_TYPE(decltype(io.size()), unsigned int);
```
? If so, I think we need to test that (i.e. please add that line here).
================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:16-17
#include <cassert>
+#include <algorithm>
#include <string>
----------------
`a` < `c`; please swap lines 16 and 17.
(And traditionally we'd put `<string>` first of all, because this test is testing `<string>`, but I guess we've already lost that pattern in this file, which is not necessarily awful.)
================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp:14-21
// template<class Context, class... Args>
// struct format-arg-store { // exposition only
// array<basic_format_arg<Context>, sizeof...(Args)> args;
// };
//
// Note more testing is done in the unit test for:
// template<class Visitor, class Context>
----------------
@Mordante should this entire file move to `test/libcxx/`?
================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg.store/make_format_args.pass.cpp:12
// TODO FMT Evaluate gcc-11 status
// UNSUPPORTED: gcc-11
----------------
@Mordante should this entire file also move to `test/libcxx/`?
================
Comment at: libcxx/test/std/utilities/format/format.arguments/format.arg.store/make_wformat_args.pass.cpp:13
// TODO FMT Evaluate gcc-11 status
// UNSUPPORTED: gcc-11
----------------
@Mordante: and finally, should //this// file also move to `test/libcxx/`?
================
Comment at: libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/check_arg_id.pass.cpp:45
"argument numbering mode") == 0);
+ (void) e;
return;
----------------
You could mark `e` as `[[maybe_unused]]` instead, since this is a C++20 test.
(I'm personally still not sold on `[[maybe_unused]]`, but you //have// been using it elsewhere in this PR, and we do use it lots of places by now already.)
================
Comment at: libcxx/test/std/utilities/format/format.functions/format.pass.cpp:70-74
#else
- (void)what;
(void)fmt;
(void)sizeof...(args);
#endif
+ (void)what;
----------------
Let's move all of this outside the `#else`, and then we don't need an `#else`.
================
Comment at: libcxx/test/std/utilities/format/format.functions/format_tests.h:167
"A format-spec arg-id replacement exceeds the maximum supported value",
- STR("hello {:{}}"), world, -1u);
+ STR("hello {:{}}"), world, ~0u);
check_exception("Argument index out of bounds", STR("hello {:{}}"), world);
----------------
FWIW, I strongly prefer `-1whatever` over `~0whatever` for readability. MSVC really warns on `-1u`? What about `unsigned(-1)` or simply `0xFFFFFFFF`?
================
Comment at: libcxx/test/std/utilities/format/format.functions/vformat.locale.pass.cpp:39-41
+ TEST_IGNORE_NODISCARD
+ std::vformat(std::locale(), fmt,
+ std::make_format_args<context_t<CharT>>(args...));
----------------
As remarked above, personally I think we should just use `(void)` casts throughout; but either way, please don't break the line after a cast, that's just weird. :p
```
TEST_IGNORE_NODISCARD std::vformat(std::locale(), fmt,
std::make_format_args<context_t<CharT>>(args...));
```
or
```
(void) std::vformat(std::locale(), fmt,
std::make_format_args<context_t<CharT>>(args...));
```
(and if clang-format is complaining, which it shouldn't, then `arc --no-lint`)
================
Comment at: libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/explicit_const_optional_U.pass.cpp:78
friend constexpr bool operator==(const Z& x, const Z& y) {return x.i_ == y.i_;}
};
----------------
Please remove `constexpr` here, too.
================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:383
struct Callable {
- bool operator()();
+ bool operator()() { std::abort(); }
};
----------------
`assert(false);`, surely!
Also, by comparison with the next file, I suspect this was meant to be a const member function.
================
Comment at: libcxx/test/support/charconv_test_helpers.h:111
// doesn't modify data beyond r.ptr.
- std::iota(buf, buf + sizeof(buf), 1);
+ std::iota(buf, buf + sizeof(buf), char{1});
r = to_chars(buf, buf + sizeof(buf), v, args...);
----------------
`char(1)` plz.
Also, //really//, MSVC? It's warning... about treating `1` as a char? That's a little aggressive, yeah?
If MSVC is giving the //same// warning in
- `charconv_test_helpers.h`
- `test_constexpr_container.h`
- `span.cons/initializer_list.pass.cpp`
- `vector/access.pass.cpp`
- `bit/byteswap.pass.cpp`
then I think you should consider just globally disabling that warning when running libc++ tests. If it's all subtly different warnings, though, then that's not so attractive an option.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D116878/new/
https://reviews.llvm.org/D116878
More information about the libcxx-commits
mailing list