[libcxx-commits] [PATCH] D132327: [libc++] Implement P2445R1 (`std::forward_like`)

[Igor Zhukov via Phabricator via libcxx-commits]

fsb4000 marked an inline comment as done.
fsb4000 added inline comments.


================
Comment at: libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.cpp:8
+
+using namespace std;
+
----------------
philnik wrote:
> I guess this is inherited from the MSVC testing style?
Yes, MSVC using this style.


================
Comment at: libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.cpp:67
+
+struct NoCtorCopyMove {
+  NoCtorCopyMove() = delete;
----------------
philnik wrote:
> Are these changes in the upstream MSVC test? If not, I'd rather have them in a separate test to reduce divergence from upstream.
I will create a PR to the MSVC test after this patch will be landed.
I'm almost sure they will merge it without issues.


================
Comment at: libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.cpp:73
+
+using NCCM = NoCtorCopyMove;
+
----------------
philnik wrote:
> Why do you use an alias here? IMO it makes the test really hard to read.
Ok, I will not use the alias.


================
Comment at: libcxx/test/std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp:12
+// Includes Microsoft's test that tests the entire header.
+
+#include "test.cpp"
----------------
philnik wrote:
> fsb4000 wrote:
> > huixie90 wrote:
> > > Is this test supposed to be running on gcc/clang? I am slightly confused because it is in "msvc" folder
> > Yes, all configurations should be tested.
> > 
> > I used https://github.com/llvm/llvm-project/tree/main/libcxx/test/std/utilities/charconv/charconv.msvc as a naming example.
> > 
> > I opened `gcc 12/ C++ latest` log: https://buildkite.com/llvm-project/libcxx-ci/builds/13210#0182deee-3853-484d-a646-79afa1e27420 and I found:
> > 
> > ```
> > ... cfg.in :: std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp
> >  98% [=========================================================--(B](B ETA: 00:00:04
> > ```
> > and `C++2b` log: https://buildkite.com/llvm-project/libcxx-ci/builds/13210#0182deee-3841-41d9-8725-06f9a249e560
> > 
> > ```
> > ... cfg.in :: std/utilities/utility/forward_like/forward_like.msvc/test.pass.cpp
> >  98% [=========================================================--(B](B ETA: 00:00:08
> > ```
> This way to include the test seems quite weird to me. Why can't we just name the MSVC test `test.pass.cpp`?
Sure, we can.
I did similar to https://github.com/llvm/llvm-project/blob/main/libcxx/test/std/utilities/charconv/charconv.msvc/test.pass.cpp

If we name the test `test.pass.cpp` should I add LLVM licence comment too?


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

https://reviews.llvm.org/D132327