[libcxx-commits] [PATCH] D126971: [libc++] Implements Unicode grapheme clustering

Wed Jul 20 08:53:05 PDT 2022

Mordante marked 2 inline comments as done.
Mordante added a comment.

Thanks for the review!

================
Comment at: libcxx/utils/generate_extended_grapheme_cluster_table.py:1
+#!/usr/bin/env python
+# ===----------------------------------------------------------------------===##
----------------
ldionne wrote:
> Mordante wrote:
> > ldionne wrote:
> > > According to what you just said during live review, https://www.unicode.org/Public/12.0.0/ucd/auxiliary/GraphemeBreakTest.txt changes from time to time (roughly once a year). Consequently, I think we'll want to make it as easy as possible to update. As discussed, we also don't want to get automatic updates under our feet without manual intervention, because the rules might need to change, etc. Hence, I would suggest:
> > > 
> > > 1. We download the tables that you use to generate headers and tests and check them into libc++ in a reasonable location. Preferably in a subdirectory with a `README.md` that explains what those are and where to download them from.
> > > 2. We add the running of the generation scripts to `libcxx-generate-files`.
> > > 
> > > That way, the workflow for updating those files is simply to re-download the latest tables online, re-generate the headers+tests using `libcxx-generate-files`, and check that in. Furthermore, if we ever forget to re-generate the headers and tests, the CI will tell us through our existing mechanisms.
> > I've given this some more thought. Instead of making these changes in this commit I will work on a follow-up commit to make these changes. I don't see the easy way to update as a must-have for LLVM-15 since I don't intend to update these tables in the release branch. That way there's less risk for this patch to miss the branching point.
> That sounds reasonable, but let's not forget to do this. It's really important to have an easy way to keep those up-to-date, or they will become stale in no time.
I've already created D129668, which most likely needs more work.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126971