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

[Casey Carter via Phabricator via libcxx-commits]

CaseyCarter marked 13 inline comments as done.
CaseyCarter added inline comments.


================
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;
 }
----------------
Quuxplusone wrote:
> 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?...
I'll take a quick stab at it. 


================
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());
----------------
Quuxplusone wrote:
> 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`?)
Changing the return type works. 


================
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;
----------------
Quuxplusone wrote:
> 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`.
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.


================
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);
----------------
Quuxplusone wrote:
> Here and above, can we get away with `char(0xCD)`, `int16_t(0xCDEF)`, `int8_t(0xAB)`? Those would be easier on the eyes.
Sadly, no - `int8_t(0xAB)`, `(int8_t)0xAB`, and `INT8_C(0xAB)` all produce the same truncation warning. 


================
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()));
   }
----------------
Quuxplusone wrote:
> 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).
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.


================
Comment at: libcxx/test/std/strings/basic.string/string.capacity/resize_and_overwrite.pass.cpp:16-17
 
 #include <cassert>
+#include <algorithm>
 #include <string>
----------------
Quuxplusone wrote:
> `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.)
Alphabet is hard.


================
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 wrote:
> 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.
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.


================
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);
----------------
Mordante wrote:
> 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?
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." 


================
Comment at: libcxx/test/std/utilities/variant/variant.visit/visit.pass.cpp:383
     struct Callable {
-      bool operator()();
+      bool operator()() { std::abort(); }
     };
----------------
Quuxplusone wrote:
> `assert(false);`, surely!
> Also, by comparison with the next file, I suspect this was meant to be a const member function.
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.)


================
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...);
----------------
Quuxplusone wrote:
> `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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116878