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

Wed Jul 20 08:25:27 PDT 2022

ldionne accepted this revision.
ldionne added inline comments.

================
Comment at: libcxx/utils/generate_extended_grapheme_cluster_table.py:1
+#!/usr/bin/env python
+# ===----------------------------------------------------------------------===##
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126971