[libcxx-commits] [PATCH] D116878: [libcxx][test] Add missing includes and suppress warnings

[Mark de Wever via Phabricator via libcxx-commits]

Mordante accepted this revision.
Mordante added a comment.
This revision is now accepted and ready to land.

Thanks for the cleanups!
LGTM modulo some small nits.



================
Comment at: libcxx/test/std/containers/sequences/vector/access.pass.cpp:51
     try {
-        c.at(n);
+        TEST_IGNORE_NODISCARD c.at(n);
         assert(false);
----------------
Quuxplusone wrote:
> 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.)
I like this `TEST_IGNORE_NODISCARD` macro, but I wasn't aware it existed. Not too fond of `_LIBCPP_UNUSED_VAR(x)`.


================
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>
----------------
Quuxplusone wrote:
> @Mordante should this entire file move to `test/libcxx/`?
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.


================
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
 
----------------
Quuxplusone wrote:
> @Mordante should this entire file also move to `test/libcxx/`?
No it tests whether the call to `std::make_format_args` is valid. Its return type is exposition only hence the `LIBCPP_ASSERT`s. 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`.


================
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;
----------------
Quuxplusone wrote:
> Let's move all of this outside the `#else`, and then we don't need an `#else`.
Please do the same for similar cases in the other tests.


================
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);
----------------
Quuxplusone wrote:
> FWIW, I strongly prefer `-1whatever` over `~0whatever` for readability. MSVC really warns on `-1u`? What about `unsigned(-1)` or simply `0xFFFFFFFF`?
I prefer `unsigned(-1)`, or is that also a warning for MSVC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116878