[libcxx-commits] [PATCH] D145520: [libc++][test] Uses qualified std::uint32_t.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

Thanks for the quick reviews!

In D145520#4175998 <https://reviews.llvm.org/D145520#4175998>, @ldionne wrote:

> In D145520#4175977 <https://reviews.llvm.org/D145520#4175977>, @philnik wrote:
>
>> Is there a reason you don't qualify all the integer aliases in a single patch? Also, should we maybe start to enforce a few clang-tidy checks on the tests, so we can avoid regressing on this? It's probably not too hard to add a check.
>
> Mark and I discussed this offline and we decided to start with a small patch to evaluate how that was going to behave with downstream merging. Other patches will touch things like `size_t` (so 1000+ files in the test suite). Seeing the reasonable impact of this one, I wouldn't be opposed to doing this for other types like `int64_t` & friends in this patch, if it keeps the scope similar. But I'm also OK with separate patches.

As discussed with Louis we don't need to check it. Once we have the module std the CI will check it. So yes we might regress between now and that time, but at least then the changes will be small.

For the first patch I think this is a nice size, for the first step. I feel that doing all other 'u?int(_least|_fast|)\d\d?_t` types can be done in the one patch.

PS I wouldn't mind to clang-tidy our tests too.



================
Comment at: libcxx/test/std/input.output/iostream.format/output.streams/ostream.formatted/ostream.inserters.arithmetic/minus1.pass.cpp:19
 // operator<<( int32_t val);
-// operator<<(uint32_t val);
+// operator<<(std::uint32_t val);
 // operator<<( int64_t val);
----------------
ldionne wrote:
> Technically, I think this doesn't need to change since the synopsis in the standard is based on the fact that we're already inside namespace `std`.
Maybe I should look at avoiding updating comments. For the synopsis it indeed feels wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145520