[PATCH] D55308: Implement the second part of P0482 (char8_t)

[Louis Dionne via Phabricator]

ldionne added a comment.

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.

2. 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.



================
Comment at: test/std/language.support/support.limits/support.limits.general/istream.version.pass.cpp:18
+
+#include <string_view>
+#include <cassert>
----------------
This should be `<istream>`


================
Comment at: test/std/language.support/support.limits/support.limits.general/limits.version.pass.cpp:18
+
+#include <string_view>
+#include <cassert>
----------------
`<limits>`


================
Comment at: test/std/language.support/support.limits/support.limits.general/locale.version.pass.cpp:18
+
+#include <string_view>
+#include <cassert>
----------------
`<locale>`


================
Comment at: test/std/language.support/support.limits/support.limits.general/ostream.version.pass.cpp:18
+
+#include <string_view>
+#include <cassert>
----------------
`<ostream>`


================
Comment at: test/std/strings/basic.string.hash/enabled_hashes.pass.cpp:26
     test_hash_enabled_for_type<std::wstring>();
+#if defined(__cpp_lib_char8_t) && __cpp_lib_char8_t >= 201811L
+    test_hash_enabled_for_type<std::u8string>();
----------------
Why would the value ever be less than `201811L`? Genuinely asking.


================
Comment at: test/std/strings/basic.string.literals/literal.pass.cpp:19
+//	This is changed by P0482 to return a std::u8string - re-enable when we implement that.
+#if defined(__cpp_lib_char8_t) && __cpp_lib_char8_t >= 201811L
+    typedef std::u8string u8string;
----------------
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?


================
Comment at: test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char8_t/assign2.pass.cpp:24
+    char8_t c = u'1';
+    std::char_traits<char8_t>::assign(c, u'a');
+    return c == u'a';
----------------
This would have to be guarded in a `#if defined(__cpp_lib_char8_t) && __cpp_lib_char8_t >= 201811L` too, I think.


================
Comment at: test/std/strings/char.traits/char.traits.specializations/char.traits.specializations.char8_t/find.pass.cpp:25
+    constexpr const char8_t *p = u8"123";
+    return std::char_traits<char8_t>::find(p, 3, u8'1') == p
+        && std::char_traits<char8_t>::find(p, 3, u8'2') == p + 1
----------------
This needs to be guarded in a `#if` too?


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

https://reviews.llvm.org/D55308