From: Adhemerval Zanella <adhemerval.zanella@linaro.org>
To: Tom Honermann <tom@honermann.net>
Cc: libc-alpha@sourceware.org
Subject: Re: [PATCH v4 0/3] C++20 P0482R6 and C2X N2653: char8_t, mbrtoc8(), and c8rtomb()
Date: Mon, 4 Jul 2022 16:08:45 -0300 [thread overview]
Message-ID: <AF8CE5E7-ECC8-4BC0-A19E-6A4A5C70AECC@linaro.org> (raw)
In-Reply-To: <20220630125215.6052-1-tom@honermann.net>
> On 30 Jun 2022, at 09:52, Tom Honermann via Libc-alpha <libc-alpha@sourceware.org> wrote:
>
> This series of patches provides the following:
> - A fix for BZ #25744 [1].
> - Implementations of the mbrtoc8 and c8rtomb functions adopted for
> C++20 via WG21 P0482R6 [2] and for C2X via WG14 N2653 [3].
> - A char8_t typedef as adopted for C2X via WG14 N2653 [3].
>
> The fix for BZ #25744 [1] is included in this patch series because the tests
> for mbrtoc8 and c8rtomb depend on it for exercising the special case where a
> pair of Unicode code points is converted to/from a single double byte sequence.
> Such conversion cases exist for Big5-HKSCS.
>
> N2653 was adopted by WG14 for C2X and wording is present in the N2912 revision
> of the C2X working draft [4]. This patch series enables the new declarations
> in C2X mode and when _GNU_SOURCE is defined.
>
> This patch series addresses feedback provided in response to the prior
> submission series starting at [5]. All feedback was addressed except as noted
> below:
> - I removed the previously proposed wcsmbs/test-char8-type.c test as redundant
> per feedback. I prototyped extending the c++-types.data test as suggested,
> but that would have introduced a C++20 dependency. The test demonstrated that
> the char16_t and char32_t types were mapped to 'Ds' and 'Di' as expected, but
> char8_t got mapped to 'h' rather than 'Du'. It might be worth extending this
> test in the future if/when C++20 support becomes a minimal requirement.
> - I did not switch the iconvdata/tst-iconv-big5-hkscs-to-2ucs4.c test to use
> TEST_COMPARE in favor of the existing custom error handling; the current
> error messages are customized to provide a more detailed explanation of the
> error, so removing that seemed like an arguable regression. My changes retain
> consistency with the existing code. I think it is reasonable to switch to
> TEST_COMPARE, but I think that should be done separately from this change and
> separately motivated.
> - I did not change the c8rtomb() and mbrtoc8() implementations to use thread
> local storage when PS is null; the implementations continue to use static
> storage consistent with c16rtomb(), mbrtoc16(), c32rtomb(), and mbrtoc32().
> I think it is reasonable to switch all of these functions to use thread
> local storage, but I think such a change should be done consistently across
> all of them and should be separately motivated.
>
> Thank you to Joseph Myers, Carlos O'Donell, Adhemerval Zanella, and Florian
> Weimer for prior reviews of this patch series.
>
> Tested on Linux x86_64.
>
> [1]: Bug 25744
> "mbrtowc with Big5-HKSCS returns 2 instead of 1 when consuming the
> second byte of certain double byte characters"
> https://sourceware.org/bugzilla/show_bug.cgi?id=25744
>
> [2]: WG21 P0482R6
> "char8_t: A type for UTF-8 characters and strings (Revision 6)"
> https://wg21.link/p0482r6
>
> [3]: WG14 N2653
> "char8_t: A type for UTF-8 characters and strings (Revision 1)"
> http://www.open-std.org/jtc1/sc22/wg14/www/docs/n2653.htm
>
> [4]: WG14 N2912
> "Programming languages — C working draft — June 8, 2022"
> https://www.open-std.org/jtc1/sc22/wg14/www/docs/n2912.pdf
>
> [5]: libc-alpha mailing list
> "[PATCH 0/3]: C++20 P0482R6 and C2X N2653: support for char8_t, mbrtoc8(), and c8rtomb()."
> https://sourceware.org/pipermail/libc-alpha/2022-February/136723.html
Hi Tom,
Patches look good in general, just some really minor style issues I
found in last review. I have fixed them and if you are ok I can
install them, I have create a branch so you can check [1].
[1] https://sourceware.org/git/?p=glibc.git;a=shortlog;h=refs/heads/azanella/mbrtoc8-c8rtomb
next prev parent reply other threads:[~2022-07-04 19:08 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-30 12:52 Tom Honermann
2022-06-30 12:52 ` [PATCH v4 1/3] gconv: Correct Big5-HKSCS conversion to preserve all state bits. [BZ #25744] Tom Honermann
2022-07-04 18:16 ` Adhemerval Zanella
2022-06-30 12:52 ` [PATCH v4 2/3] stdlib: Implement mbrtoc8(), c8rtomb(), and the char8_t typedef Tom Honermann
2022-07-04 18:33 ` Adhemerval Zanella
2022-07-19 21:08 ` Joseph Myers
2022-07-20 12:04 ` Adhemerval Zanella Netto
2022-07-20 13:54 ` Florian Weimer
2022-07-20 14:31 ` Adhemerval Zanella Netto
2022-07-20 15:05 ` Florian Weimer
2022-07-20 16:53 ` Tom Honermann
2022-07-20 16:47 ` Tom Honermann
2022-07-21 19:22 ` Adhemerval Zanella Netto
2022-07-21 20:51 ` Tom Honermann
2022-07-21 20:56 ` Adhemerval Zanella Netto
2022-07-22 5:24 ` Tom Honermann
2022-07-22 11:21 ` Adhemerval Zanella Netto
2022-07-22 14:15 ` Adhemerval Zanella Netto
2022-07-22 17:00 ` Tom Honermann
2022-07-22 17:01 ` Adhemerval Zanella Netto
2022-07-24 4:46 ` Tom Honermann
2022-06-30 12:52 ` [PATCH v4 3/3] stdlib: Tests for " Tom Honermann
2022-07-04 18:58 ` Adhemerval Zanella
2022-07-04 19:08 ` Adhemerval Zanella [this message]
2022-07-06 3:27 ` [PATCH v4 0/3] C++20 P0482R6 and C2X N2653: char8_t, mbrtoc8(), and c8rtomb() Tom Honermann
2022-07-06 12:23 ` Adhemerval Zanella
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AF8CE5E7-ECC8-4BC0-A19E-6A4A5C70AECC@linaro.org \
--to=adhemerval.zanella@linaro.org \
--cc=libc-alpha@sourceware.org \
--cc=tom@honermann.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).