This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] adds Microsoft/STL tests for P0898R3
AbandonedPublic

Authored by cjdb on Apr 2 2021, 9:50 AM.

Details

Summary

It was agreed upon early in the process of adding <concepts> to libc++
that Microsoft's STL tests should be transplanted for cross-pollination.
This patch honours that.

Since these tests have already been reviewed by the folks working on
Microsoft/STL, libc++ reviewers should consult
libcxx/test/msvc-stl/README.md.

Depends on D96477.

Diff Detail

Event Timeline

cjdb requested review of this revision.Apr 2 2021, 9:50 AM
cjdb created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2021, 9:50 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Seems fine to me, as long as
(1) @ldionne is okay with the licensing terms (and should they perhaps be repeated in the README?)
(2) running ./bin/llvm-lit ../libcxx/test/ picks up these tests and does the Right Thing with them

libcxx/test/msvc-stl/README.md
10

You also added // clang-format on in some places. I recommend deleting it; these files will never be subjected to clang-format.

Concerning the formatting. It would probably be easier to create a .clang-format config file in libcxx/test/msvc-stl/ with DisableFormat: true instead of putting format comments.

libcxx/test/msvc-stl/README.md
11

I really like that we use these Microsoft tests!
I would like to give some thought how we'll do this since I like to use this as a precedent. D70631 also contains Microsoft tests, but uses a different directory and a different approach to including the tests. (This patch will be done before D70631 so best to use this one to discuss it.)

libcxx/test/msvc-stl/P0898R3_concepts/invocable_cc.hpp
1

I like the tests in this separate directory.

libcxx/test/msvc-stl/README.md
1

Please add some information about how to add Microsoft unit tests to libc++ in docs/Contributing.rst, which then refers to this document for the details.

9

What I did in D70631 was a slightly different approach. I kept test.cpp and added test.pass.cpp The new file consists of:

// Copyright block

// Lit directives

#include "test.cpp"

This change hasn't be reviewed so I don't know whether it's acceptable.
I choose this approach to the Microsoft files don't contain libc++ lit directives and there would be no need to rename the file.

WDYT of this approach?

I think this is a great idea, but I also think we need to think about how we're going to handle this in the long term. In particular, I'm a bit wary of updates to these files.

How difficult would it be to write a script that imports those tests from msvc to libc++? This way we could run it periodically and stay up to date? In that spirit, making the smallest number of alterations to the files seems like a must, so I'd welcome using a .clang-format at the root of the directory and pretty much anything else that can move us towards that goal.

I don't see any issue w.r.t. licensing since those appear to be licensed under the appropriate license. I don't think it matters who the copyright holder is.

ldionne added inline comments.Apr 7 2021, 11:43 AM
libcxx/test/msvc-stl/README.md
9

I think that's quite interesting, and yes indeed I think it works. I like that. If we could put all the test files under some subdirectory to make it clear they are not from libc++, I think that's even better.

cjdb abandoned this revision.Jan 18 2022, 11:54 AM
libcxx/test/msvc-stl/P0898R3_concepts/invocable_cc.hpp