This is an archive of the discontinued LLVM Phabricator instance.

[libcxx][test] Add missing includes and suppress warnings
ClosedPublic

Authored by CaseyCarter on Jan 9 2022, 12:25 AM.

Details

Summary

... from testing with MSVC's STL. Mostly truncation warnings and variables that are only used in LIBCPP_ASSERT.

Diff Detail

Event Timeline

CaseyCarter requested review of this revision.Jan 9 2022, 12:25 AM
CaseyCarter created this revision.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptJan 9 2022, 12:25 AM

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
libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable.pass.cpp
161–164

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

libcxx/test/std/containers/sequences/vector/access.pass.cpp
51

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

libcxx/test/std/containers/views/span.cons/initializer_list.pass.cpp
24–31

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?)

libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp
37

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.

libcxx/test/std/numerics/bit/byteswap.pass.cpp
47–48
76

Here and above, can we get away with char(0xCD), int16_t(0xCDEF), int8_t(0xAB)? Those would be easier on the eyes.

libcxx/test/std/ranges/range.factories/range.iota.view/size.pass.cpp
67–69

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

libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp
17–18

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

libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp
14–21

@Mordante should this entire file move to test/libcxx/?

libcxx/test/std/utilities/format/format.arguments/format.arg.store/make_format_args.pass.cpp
12

@Mordante should this entire file also move to test/libcxx/?

libcxx/test/std/utilities/format/format.arguments/format.arg.store/make_wformat_args.pass.cpp
13

@Mordante: and finally, should this file also move to test/libcxx/?

libcxx/test/std/utilities/format/format.formatter/format.parse.ctx/check_arg_id.pass.cpp
45

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

libcxx/test/std/utilities/format/format.functions/format.pass.cpp
70–74

Let's move all of this outside the #else, and then we don't need an #else.

libcxx/test/std/utilities/format/format.functions/format_tests.h
168

FWIW, I strongly prefer -1whatever over ~0whatever for readability. MSVC really warns on -1u? What about unsigned(-1) or simply 0xFFFFFFFF?

libcxx/test/std/utilities/format/format.functions/vformat.locale.pass.cpp
39–41

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)

libcxx/test/std/utilities/optional/optional.object/optional.object.ctor/explicit_const_optional_U.pass.cpp
78

Please remove constexpr here, too.

libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
383

assert(false);, surely!
Also, by comparison with the next file, I suspect this was meant to be a const member function.

libcxx/test/support/charconv_test_helpers.h
111

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.

Mordante accepted this revision.Jan 10 2022, 9:32 AM

Thanks for the cleanups!
LGTM modulo some small nits.

libcxx/test/std/containers/sequences/vector/access.pass.cpp
51

I like this TEST_IGNORE_NODISCARD macro, but I wasn't aware it existed. Not too fond of _LIBCPP_UNUSED_VAR(x).

libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp
14–21

Nope the file can actually be removed. I've a not-yet-ready-for-review patch to improve the format-arg-store and there I remove the file. The removal can also be done in this commit or later.

libcxx/test/std/utilities/format/format.arguments/format.arg.store/make_format_args.pass.cpp
12

No it tests whether the call to std::make_format_args is valid. Its return type is exposition only hence the LIBCPP_ASSERTs. In the aforementioned patch this file will only contain std::make_format_args(42, nullptr, false, 1.0);.

Same story for make_wformat_args.pass.cpp.

libcxx/test/std/utilities/format/format.functions/format.pass.cpp
70–74

Please do the same for similar cases in the other tests.

libcxx/test/std/utilities/format/format.functions/format_tests.h
168

I prefer unsigned(-1), or is that also a warning for MSVC?

This revision is now accepted and ready to land.Jan 10 2022, 9:32 AM

Address review comments.

CaseyCarter marked 13 inline comments as done.Jan 13 2022, 5:31 PM
CaseyCarter added inline comments.
libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable.pass.cpp
161–164

I'll take a quick stab at it.

libcxx/test/std/containers/views/span.cons/initializer_list.pass.cpp
24–31

Changing the return type works.

libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp
37

You're preaching to the choir: _any_ source change is better than a #pragma. Unfortunately, this int-to-double conversion isn't the source; MSVC believes integral-to-floating conversions are generally benign. It's actually diagnosing the writes to the insert iterator on lines 45 and 46. These call insert_iterator's operator= that takes an int&& with a temporary int converted from double. As the comment on 45 makes clear, the intent is to verify that the conversion occurs.

Now that I think about it, I can correct the warning by using a simple program-defined value type instead of int, since conversions to user-defined-types are not narrowing.

libcxx/test/std/numerics/bit/byteswap.pass.cpp
76

Sadly, no - int8_t(0xAB), (int8_t)0xAB, and INT8_C(0xAB) all produce the same truncation warning.

libcxx/test/std/ranges/range.factories/range.iota.view/size.pass.cpp
67–69

The root cause here is that C1XX won't realize that the RHS is a constant expression, evaluate it, and notice that it's positive and therefore preserved by conversion to unsigned int for the comparison. The other test cases are all comparisons with literals and thus just simple enough to slip by this pre-constexpr-era warning code. Actually, I can probably hack this by storing numeric_limits<int>::max() in a local constexpr variable - I'll do that.

libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp
17–18

Alphabet is hard.

libcxx/test/std/utilities/format/format.arguments/format.arg.store/class.pass.cpp
14–21

I'd feel weird removing this wholesale without adding make_format_args coverage elsewhere. I'll go ahead and make this change, and you all can have your way with the file after.

libcxx/test/std/utilities/format/format.functions/format_tests.h
168

Looks like we're happy with unsigned(-1). Yes, it really warns - something like "Hey, dummy, -meow isn't really negative since meow is unsigned."

libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp
383

Changed to assert, added a bogus return statement to silence warnings. Did I mention how much it bothers me that assert will _never_ be [[noreturn]] in the UCRT? (And don't call me Shirley.)

libcxx/test/support/charconv_test_helpers.h
111

The warning is for the narrowing conversion from iota's int accumulator to char when assigning through buf. And no, I think those cases represent at least four different warnings.

CaseyCarter marked 6 inline comments as done.Jan 13 2022, 5:33 PM

Actually, I'll go ahead and merge - shout if you dislike the general refactoring in libcxx/test/std/concepts/concepts.lang/concept.swappable/swappable.pass.cpp, and I'll followup.

This revision was landed with ongoing or failed builds.Jan 13 2022, 5:34 PM
This revision was automatically updated to reflect the committed changes.