Page MenuHomePhabricator

Implement the second part of P0482 (char8_t)
ClosedPublic

Authored by mclow.lists on Dec 4 2018, 6:01 PM.

Details

Reviewers
ldionne
EricWF
Summary

The first part was type_traits and numeric_limits (already committed)
This implements the changes to string/string/view/char_traits

I defined the feature test macro __cpp_lib_char8_t even though we don't fully support it (yet) because I want the tests to use it.

Drive-by fixes include fixing the rest of the feature-test macros which were using the never-defined symbol _TEST_STD_VER.

There's more to come on P0482, but this is a self-contained chunk.

Diff Detail

Event Timeline

mclow.lists created this revision.Dec 4 2018, 6:01 PM
mclow.lists marked 3 inline comments as done.Dec 5 2018, 9:21 AM
mclow.lists added inline comments.
test/std/strings/basic.string/string.cons/string_view_size_size_deduction.pass.cpp
79

Oops. This was supposed to be an addition, not a replacement

test/std/strings/string.view/string_view.literals/literal.pass.cpp
21

Remove this comment.

39

Tab

Two overarching comments.

  1. Maybe we could/should encapsulate the logic for checking the feature test macro in a utility header for the tests. This way, we wouldn't have to copy/paste
#if TEST_STD_VER > 17
  LIBCPP_ASSERT(IS_DEFINED(__cpp_lib_char8_t)); // libc++ implements this
# if defined(__cpp_lib_char8_t)
#  if __cpp_lib_char8_t < 201811L
#   error "__cpp_lib_char8_t has an invalid value"
#  endif
# endif
#endif

so many times. Also, it would then be reasonable to have this instead:

#if TEST_STD_VER > 17
# if defined(_LIBCPP_VERSION) && !defined(__cpp_lib_char8_t)
#   error "libc++ implements this"
# endif
# if defined(__cpp_lib_char8_t)
#  if __cpp_lib_char8_t < 201811L
#   error "__cpp_lib_char8_t has an invalid value"
#  endif
# endif
#endif

This would remove the need for the IS_DEFINED macro, which is a bit hacky.

  1. Maybe we should consider having a LIT feature to represent char8_t support, that way we could mark tests as unsupported? Otherwise, I would say just assume that a compiler implementing c++2a supports char8_t and don't bother disabling the tests.
test/std/language.support/support.limits/support.limits.general/istream.version.pass.cpp
19

This should be <istream>

test/std/language.support/support.limits/support.limits.general/limits.version.pass.cpp
19

<limits>

test/std/language.support/support.limits/support.limits.general/locale.version.pass.cpp
19

<locale>

test/std/language.support/support.limits/support.limits.general/ostream.version.pass.cpp
19

<ostream>

test/std/strings/basic.string.hash/enabled_hashes.pass.cpp
26

Why would the value ever be less than 201811L? Genuinely asking.

test/std/strings/basic.string.literals/literal.pass.cpp
19

I don't understand the part of the comment that says "re-enable when we implement that". With this patch, I think defined(__cpp_lib_char8_t) && __cpp_lib_char8_t >= 201811L is true, right?

test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char8_t/assign2.pass.cpp
25

This would have to be guarded in a #if defined(__cpp_lib_char8_t) && __cpp_lib_char8_t >= 201811L too, I think.

test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char8_t/find.pass.cpp
26

This needs to be guarded in a #if too?

mclow.lists marked 2 inline comments as done.Dec 5 2018, 2:01 PM
  1. Maybe we should consider having a LIT feature to represent char8_t support, that way we could mark tests as unsupported?

I thought about that, but I didn't want to start a trend of adding lit features (because there would be several).

Otherwise, I would say just assume that a compiler implementing c++2a supports char8_t and don't bother disabling the tests.

This is demonstrably untrue.
clang 7.0, for example, supports -std=c++2a but not char8_t.

test/std/strings/basic.string.literals/literal.pass.cpp
19

I have removed this.

Two overarching comments.

  1. Maybe we could/should encapsulate the logic for checking the feature test macro in a utility header for the tests. This way, we wouldn't have to copy/paste ` #if TEST_STD_VER > 17 LIBCPP_ASSERT(IS_DEFINED(__cpp_lib_char8_t)); // libc++ implements this
  2. if defined(__cpp_lib_char8_t)
  3. if __cpp_lib_char8_t < 201811L
  4. error "__cpp_lib_char8_t has an invalid value"
  5. endif
  6. endif #endif ` so many times. Also, it would then be reasonable to have this instead: ` #if TEST_STD_VER > 17
  7. if defined(_LIBCPP_VERSION) && !defined(__cpp_lib_char8_t)
  8. error "libc++ implements this"
  9. endif
  10. if defined(__cpp_lib_char8_t)
  11. if __cpp_lib_char8_t < 201811L
  12. error "__cpp_lib_char8_t has an invalid value"
  13. endif
  14. endif #endif `

    This would remove the need for the IS_DEFINED macro, which is a bit tacky.

Agreed.
Though I like the formulation that I posted to livcxx-dev:

#if TEST_STD_VER > 17
# if !defined(__cpp_lib_char8_t)  
  LIBCPP_STATIC_ASSERT(false, "__cpp_lib_char8_t is not defined");
# else
#  if __cpp_lib_char8_t < 201811L
#   error "__cpp_lib_char8_t has an invalid value"
#  endif
# endif
#endif

This fails silently if the macro is not defined - for libraries other than libc++.
If other library maintainers want to add (say) a libstdc++-specific assertion, then there's a place for it.

mclow.lists marked 3 inline comments as done.Dec 5 2018, 2:52 PM
mclow.lists added inline comments.
test/std/strings/basic.string.hash/enabled_hashes.pass.cpp
26

No, but it could get bigger (if we add other char8_t stuff, and then the test for the new stuff would be:
#if defined(__cpp_lib_char8_t) && __cpp_lib_char8_t >= 201903L, so I just used the same pattern.

Addressed Louis' comments.

mclow.lists marked 4 inline comments as done.Dec 5 2018, 2:55 PM
mclow.lists added inline comments.
test/std/language.support/support.limits/support.limits.general/ostream.version.pass.cpp
19

Whoops. Should be <ostream> Will fix before commit.

ldionne requested changes to this revision.Dec 6 2018, 1:46 PM

Agreed.
Though I like the formulation that I posted to livcxx-dev:

#if TEST_STD_VER > 17
# if !defined(__cpp_lib_char8_t)  
  LIBCPP_STATIC_ASSERT(false, "__cpp_lib_char8_t is not defined");
# else
#  if __cpp_lib_char8_t < 201811L
#   error "__cpp_lib_char8_t has an invalid value"
#  endif
# endif
#endif

This fails silently if the macro is not defined - for libraries other than libc++.
If other library maintainers want to add (say) a libstdc++-specific assertion, then there's a place for it.

I'd err on the side of what Richard said and always assert, but otherwise I'm fine with something like this. But it would be nice to extract this check into a separate utility header to avoid duplication.

test/std/language.support/support.limits/support.limits.general/limits.version.pass.cpp
19

Still wrong?

test/std/language.support/support.limits/support.limits.general/locale.version.pass.cpp
19

Still wrong?

This revision now requires changes to proceed.Dec 6 2018, 1:46 PM

Agreed.
Though I like the formulation that I posted to livcxx-dev:

#if TEST_STD_VER > 17
# if !defined(__cpp_lib_char8_t)  
  LIBCPP_STATIC_ASSERT(false, "__cpp_lib_char8_t is not defined");
# else
#  if __cpp_lib_char8_t < 201811L
#   error "__cpp_lib_char8_t has an invalid value"
#  endif
# endif
#endif

This fails silently if the macro is not defined - for libraries other than libc++.
If other library maintainers want to add (say) a libstdc++-specific assertion, then there's a place for it.

I'd err on the side of what Richard said and always assert, but otherwise I'm fine with something like this.

I would not; consider the test algorithm.version.pass.cpp, which should check for six different flags. Do you want that test to fail until libc++ implements all six of them? I do not. I want the tests to check for all the flags that libc++ has defined, and make sure that they are (a) defined, and (b) have a sane value. Things that we haven't implemented yet should not cause test failures.

But it would be nice to extract this check into a separate utility header to avoid duplication.

I don't think that's a good idea; that header would quickly grow into all the tests for all the feature flags.
But I'm willing to discuss that.

But it would be nice to extract this check into a separate utility header to avoid duplication.

I don't think that's a good idea; that header would quickly grow into all the tests for all the feature flags.
But I'm willing to discuss that.

Or at least all the tests for all the feature flags that have to support more than one header.

mclow.lists marked 4 inline comments as done.Dec 7 2018, 1:35 PM
mclow.lists added inline comments.
test/std/language.support/support.limits/support.limits.general/limits.version.pass.cpp
19

Fixed locally.

test/std/language.support/support.limits/support.limits.general/locale.version.pass.cpp
19

Fixed locally.

I would not; consider the test algorithm.version.pass.cpp, which should check for six different flags. Do you want that test to fail until libc++ implements all six of them? I do not. I want the tests to check for all the flags that libc++ has defined, and make sure that they are (a) defined, and (b) have a sane value. Things that we haven't implemented yet should not cause test failures.

I disagree here (in an ideal world). Otherwise, what we have is not a C++ Standard conformance test suite, it's a libc++ test suite only. But okay, that's part of a larger discussion anyway.

But it would be nice to extract this check into a separate utility header to avoid duplication.

I don't think that's a good idea; that header would quickly grow into all the tests for all the feature flags.
But I'm willing to discuss that.

That would be one header per feature-test macro. Maybe that's too much, but I dislike the replication. Leave it as-is, we can always change later when we define more feature test macros if it makes sense then.

include/__string
404

_LIBCPP_INLINE_VISIBILITY here and below.

484

nullptr?

include/string
4173

I hate feature test macros already. This should just be #if _LIBCPP_STD_VER > 17... Can't we take the slightly more aggressive stance of "if you're trying to use libc++ as a c++20 library but your compiler does not support c++20, you're out of luck"?

mclow.lists marked 5 inline comments as done.Dec 7 2018, 5:21 PM
mclow.lists added inline comments.
include/__string
404

Right.

484

Meh. Sure.
Not going to go back and fix the other char_traits specializations in this patch, though.

include/string
4173

I hate feature test macros already.

Agreed.

Can't we take the slightly more aggressive stance of "if you're trying to use libc++ as a c++20 library but your compiler does not support c++20, you're out of luck"

We can't do that, because (for example) LLVM 7.0 claims to support C++20, but doesn't support char8_t.

In general, I agree with you, but this feature (char8_t) requires a compiler feature, or it doesn't work.

mclow.lists marked an inline comment as done.Dec 9 2018, 11:45 AM
mclow.lists added inline comments.
include/__string
404

After talking with Louis, we decided that *all* of the char_traits stuff needs _LIBCPP_INLINE_VISIBILITY pixie dust (for the case when _LIBCPP_HIDE_FROM_ABI_PER_TU is defined), but we'll handle that in a different patch.

ldionne added inline comments.Dec 10 2018, 11:04 AM
include/string
4173

We can't do that, because (for example) LLVM 7.0 claims to support C++20, but doesn't support char8_t.

Isn't that claim wrong, then? Also, surely you can't claim you implement C++20 at this point since it hasn't been voted?

I'm not trying to be rhetorical -- I'd _really_ like to avoid having to guard everything with a feature test macro because that will make the library much more complex and I'm sure it will introduce some tricky bugs. Just supporting many compilers and many C++ Standard versions is already difficult -- supporting partial implementations of a standard scares me a lot.

mclow.lists marked an inline comment as done.Dec 10 2018, 3:33 PM
mclow.lists added inline comments.
include/string
4173

I agree; we don't want to do that; and, in general, we won't need to do so.

BUT, in this case, we need a particular bit of functionality from the compiler, and the way to see if the compiler has implemented it is via a feature-test macro. In other places, we use __has_builtin or other ways of querying the compiler. This is the first place (that I know of) that we're using a feature-test macro to do so.

mclow.lists marked an inline comment as done.Dec 10 2018, 3:49 PM
mclow.lists added inline comments.
include/string
4173

Would you feel better if I defined _LIBCPP_HAS_CHAR8_T in <__config> and used that throughout?

It would look like this:

#if _LIBCPP_STD_VER > 17 && defined(__cpp_char8_t)
#define _LIBCPP_HAS_CHAR8_T
#endif
ldionne accepted this revision.Dec 10 2018, 3:53 PM

LGTM with the fixes to the tests.

include/string
4173

Ah, I see. No, I think using __cpp_char8_t is OK in that case -- I wasn't seeing it that way.

This revision is now accepted and ready to land.Dec 10 2018, 3:53 PM
mclow.lists marked an inline comment as done.Dec 10 2018, 5:23 PM
mclow.lists added inline comments.
include/string
4173

Instead I turned it around:

#if _LIBCPP_STD_VER <= 17 || !defined(__cpp_char8_t)
#define _LIBCPP_HAS_NO_CHAR8_T
#endif

That way someone can #define _LIBCPP_HAS_NO_CHAR8_T and it will all go away.

mclow.lists closed this revision.Dec 10 2018, 8:39 PM

Committed as revision 348828