[libcxx-commits] [PATCH] D109120: [libc++][NFC] Remove uses of 'using namespace std; ' in the test suite

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/test/libcxx/numerics/c.math/fdelayed-template-parsing.pass.cpp:27
-
-using namespace std;
----------------
Quuxplusone wrote:
> This can't possibly have been accidental, can it? What's the subtle point of this test?
Indeed, good catch. I did some archeology and improved the comment at the top instead.


================
Comment at: libcxx/test/std/containers/unord/unord.map/unord.map.elem/index_tuple.pass.cpp:25
+struct my_hash {
+    std::size_t operator()(const std::tuple<int, int>&) const { return 0; }
 };
----------------
Quuxplusone wrote:
> You don't need the `std::` on `size_t`. (Compare line 29, where you left it off and it was totally fine.)
> Some people put it religiously; I personally //omit// it religiously (on analogy to `int`, and because I like typing less).
> Anyway, consider making lines 25 and 29 consistent (but it doesn't matter).
When I include `<cxxxxx>`, I use the stuff in namespace `std::` as a matter of consistency.


================
Comment at: libcxx/test/std/input.output/filesystems/class.path/path.nonmember/swap.pass.cpp:35
   {
-    using namespace std; using namespace fs;
     ASSERT_NOEXCEPT(swap(p1, p2));
----------------
Quuxplusone wrote:
> This must have been intentional — trying to verify that we get no ambiguity from lookup finding both `std::swap` and `std::filesystem::swap`?? — but in this case I'm fairly sure I understand the issues involved and agree that deleting this line is fine.
> 
> Alternatively, delete `using namespace fs;` but keep `using namespace std;` — then we'd be testing that the "std::swap two-step" correctly works with `path` the same way it works with other types.
> 
> Obviously lines 41-42 should be made consistent with whatever you choose to do here.
I actually added a test that checks the swap two-step but also left this one.


================
Comment at: libcxx/test/std/strings/basic.string.literals/literal3.pass.cpp:17
+int main(int, char**) {
+    using namespace std::literals;
+    std::string foo = ""s;
----------------
Quuxplusone wrote:
> No, this one is 100% intentional. Compare literal1.cpp.
> However, please add comments to each of the files... and IMHO ideally combine them into just one file, so that it will be more obvious in the future what's supposed to be different between each file.
Hmm, right indeed -- I went a bit quickly with this patch, thanks for looking at it diligently. I fixed it, look in the next update.


================
Comment at: libcxx/test/support/any_helpers.h:93
     assert(containsType<Type>(a));
-    any_cast<Type&>(a).value = value;
+    std::any_cast<Type&>(a).value = value;
 }
----------------
Quuxplusone wrote:
> Oho. This double-`using` idiom must date back to when `std::experimental::any_cast` was a thing. We don't have to worry about that anymore, right?
Right, that was my thinking too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109120