Page MenuHomePhabricator

[libcxx] adds concept `std::convertible_to`
ClosedPublic

Authored by cjdb on Apr 11 2020, 10:23 PM.

Details

Summary

Implements parts of:

  • P0898R3 Standard Library Concepts
  • P1754 Rename concepts to standard_case for C++20, while we still can

Diff Detail

Event Timeline

cjdb created this revision.Apr 11 2020, 10:23 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 11 2020, 10:23 PM

I left mostly some nits with respect to readability. Great job

libcxx/include/concepts
189

The standard depicts those as From and To and in the type trait it is also _From and _To.

So lets be consistent and use the same naming

libcxx/test/std/concepts/lang/convertible_to.pass.cpp
21 ↗(On Diff #256820)

Does that compile without the second string argument? It is used everywhere else int eh test suite even if its empty

146 ↗(On Diff #256820)

Should we move this fundamental checks that do not rely on any Modifier to their own function? These are the most common ones so as a reader it would be easier to check them in isolation and then have the specialized function do the tricky stuff

218 ↗(On Diff #256820)

I am wondering whether it is cleaner to define those basic types above and for the constructors and then create all the others by inheritance, e.g:

struct ImplicitlyConstructibleAndConvertible : public ImplicitelyConstructible, public Convertible {
{
    using ImplicitelyConstructible::ImplicitelyConstructible;
};
261 ↗(On Diff #256820)

Newlines missing from here

cjdb marked 3 inline comments as done.Apr 17 2020, 9:27 AM
cjdb added inline comments.
libcxx/test/std/concepts/lang/convertible_to.pass.cpp
21 ↗(On Diff #256820)

C++17 makes this possible. I don't think the empty string literal is necessary, since this is a test for C++20.

146 ↗(On Diff #256820)

Yes. Do you mean fold them into a function and then call said function here, or have a references namespace?

218 ↗(On Diff #256820)

I was planning to do derived cases in a separate namespace to keep what's being tested very explicit. Do you think I'm being too granular?

miscco added inline comments.Apr 20 2020, 4:03 AM
libcxx/test/std/concepts/lang/convertible_to.pass.cpp
146 ↗(On Diff #256820)

Given that the namespace is "Fundamentals" I would move it into its own function and test it against some basic types such as int, struct A{};, enum class etc

218 ↗(On Diff #256820)

But you are doing essentially derived cases here. ImplicitlyConstructibleAndConvertible is a conjunction of ImplicitelyConstructible and ImplicitelyConvertible

Note that this is just a question whether it is actually easier to read rather than a correctness issue.

zoecarver added inline comments.
libcxx/test/std/concepts/lang/convertible_to.pass.cpp
13 ↗(On Diff #256820)

All tests should include "test_macros.h"

Ping @cjdb I knowit has been a while but are you still planning on working on this?

cjdb marked an inline comment as done.Sep 22 2020, 5:58 PM

Ping @cjdb I knowit has been a while but are you still planning on working on this?

@miscco I don't mind handing this PR off to you if you're wanting to champion the remaining bits.

@cjdb This was just a ping to see whether you are still interested.

If you say you have no time / interest I might carve some time out in the near future but it would take a while

cjdb updated this revision to Diff 321296.Wed, Feb 3, 7:24 PM
cjdb retitled this revision from WIP: Adds `std::convertible_to` to <concepts> to [libcxx] adds concept `std::convertible_to`.
cjdb edited the summary of this revision. (Show Details)
cjdb added a reviewer: ldionne.
cjdb updated this revision to Diff 321301.Wed, Feb 3, 7:49 PM
cjdb set the repository for this revision to rG LLVM Github Monorepo.

No change to one from ~10 minutes ago; trying to activate CI.

cjdb updated this revision to Diff 321537.Thu, Feb 4, 12:06 PM

Activates CI

cjdb updated this revision to Diff 321974.Sat, Feb 6, 7:26 PM

CI update

cjdb updated this revision to Diff 322012.Sun, Feb 7, 5:13 PM
cjdb edited the summary of this revision. (Show Details)

fixes CI issues

ldionne requested changes to this revision.Tue, Feb 9, 12:33 PM

This looks pretty good to me except the duplication of implementations when intrinsics are available.

More generally, have we looked into the MSVC tests for concepts? Would it make sense to import them here too or at least make sure we have the same coverage?

libcxx/include/concepts
165

As discussed offline, I would rather we use the same implementation all the time to simplify things. Let's not duplicate each implementation based on whether we can use intrinsics or not. The type traits themselves use the intrinsics, I think that's sufficient.

libcxx/test/std/concepts/lang/convertible_to.pass.cpp
13 ↗(On Diff #256820)

Technically, only include test_macros.h if you need something defined in it. The one header we always want to include is the one with nasty macros in it, but it's automatically included by the test suite harness.

This revision now requires changes to proceed.Tue, Feb 9, 12:33 PM
zoecarver added inline comments.Tue, Feb 9, 12:49 PM
libcxx/test/std/concepts/lang/convertible_to.pass.cpp
13 ↗(On Diff #256820)

I disagree, I think all tests should include this header, regardless of whether or not they actually use it. The most important reason is that sometimes someone will update a test in a few years, and add a condition with TEST_STD_VER, not realizing that TEST_STD_VER is undefined. For example, before 7fc6a55688c816f5fc1a5481ae7af25be7500356 landed, over 300 tests used TEST_STD_VER without importing test_macros.h (not to mention the 3000 others that didn't include test_macros.h without references to that macro). This means that some parts of 300 tests were silently and unintentionally not being tested.

A less compelling reason that we might want all tests to include this header is to allow portability and more complicated nasty macro testing. For example, I think some people use this test suite without lit, meaning nasty macros won't work for them. Or maybe they don't have a compiler feature that we rely on. I think having an access point into all of the tests can be super benifitial in these cases.

cjdb added a comment.Tue, Feb 9, 1:11 PM

More generally, have we looked into the MSVC tests for concepts? Would it make sense to import them here too or at least make sure we have the same coverage?

From what I've seen, MSVC seems to have one monolithic file for all the concepts, so I think there are two options for us to choose from if we want to include them:

  1. Wait till all of [concepts] is implemented, then add the MSVC test file at the root.
  2. Split the monolithic file up so it fits in with all of the libc++ tests.

The advantage of approach (1) is that it's fast, and MSVC STL contributors can painlessly update our copy of the test file whenever theirs is also updated.
However, it's a monolithic test suite, and I'm not really keen on that: I prefer my tests to be more fine-grained, but this is more work, meaning we'd lose the convenience of getting MSVC STL updates almost for free.

Which do you prefer?

More generally, have we looked into the MSVC tests for concepts? Would it make sense to import them here too or at least make sure we have the same coverage?

From what I've seen, MSVC seems to have one monolithic file for all the concepts, so I think there are two options for us to choose from if we want to include them:

  1. Wait till all of [concepts] is implemented, then add the MSVC test file at the root.
  2. Split the monolithic file up so it fits in with all of the libc++ tests.

The advantage of approach (1) is that it's fast, and MSVC STL contributors can painlessly update our copy of the test file whenever theirs is also updated.
However, it's a monolithic test suite, and I'm not really keen on that: I prefer my tests to be more fine-grained, but this is more work, meaning we'd lose the convenience of getting MSVC STL updates almost for free.

Which do you prefer?

I think we could do (1). In the meantime, let's try to make sure we have good test coverage by ourselves (as you did here).

libcxx/test/std/concepts/lang/convertible_to.pass.cpp
13 ↗(On Diff #256820)

Don't we get a warning turned into an error for using #if TEST_STD_VER > XX if TEST_STD_VER isn't defined? That would be my expectation.

I don't think I follow the logic of the argument you make, because in the same spirit, we should always be including all other headers that define any macro to make sure we don't use macros when they're undefined (and have things behave unexpectedly for hard to understand reasons). I really think it's reasonable to not include test_macros.h when it's not needed: it makes the test suite more minimal. I think I fixed the issue of silently using an undefined macro when I enabled more warnings by default last year during my work on Lit, but I could be wrong.

cjdb updated this revision to Diff 322553.Tue, Feb 9, 5:36 PM

removes \#ifdef selection

cjdb marked an inline comment as done.Tue, Feb 9, 5:36 PM
cjdb added inline comments.
libcxx/test/std/concepts/lang/convertible_to.pass.cpp
13 ↗(On Diff #256820)

std::convertible_to is a blocker for other concepts (e.g. move_constructible, common_reference_with, etc.), so I'd like to get it in ASAP.
Is it possible for this to be merged while you debate the need for including test_macros.h? I won't include it by default, but once you've reached consensus, I don't mind going back and adding it to all the concepts in one fell swoop (which I'll need to do for the already merged concepts anyway).

ldionne accepted this revision.Tue, Feb 9, 6:13 PM
ldionne added inline comments.
libcxx/test/std/concepts/lang/convertible_to.pass.cpp
13 ↗(On Diff #256820)

It was never my intention to block you on adding or not adding that include :-).

This revision is now accepted and ready to land.Tue, Feb 9, 6:13 PM
cjdb updated this revision to Diff 322568.Tue, Feb 9, 7:28 PM

renames test file

This revision was landed with ongoing or failed builds.Tue, Feb 9, 7:29 PM
This revision was automatically updated to reflect the committed changes.