This changes makes it easier to update the Unicode data files used for
the Extended Graphme Clustering as added in D126971.
Details
Details
- Reviewers
ldionne - Group Reviewers
Restricted Project - Commits
- rG130b1816c54d: [libc++] Improve updating data files.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| libcxx/utils/CMakeLists.txt | ||
|---|---|---|
| 2 | For developers and the CI a hard requirement is fine. But this might not be good for vendors. So I'm considering to make all these targets to depend on clang-format. | |
Comment Actions
Changed the properties without updating the tables.
This should result in a failing CI run.
Comment Actions
I really like this except for the clang-format requirement -- thanks!
| libcxx/utils/CMakeLists.txt | ||
|---|---|---|
| 2 | I don't think we want to add this hard requirement. Is it possible to tweak the generating scripts a tiny bit to obtain slightly (even though not perfect) output? | |
| 14 | ||
| 17–19 | I would instead use the style that we use in other scripts and hardcode the path to the file that we generate in generate_extended_grapheme_cluster_table.py. I know it's not as modular as outputting to stdout and I would never recommend that for a general purpose tool, however I'd like this to work on Windows, and I'm pretty sure the current > in the CMake target won't. If you can confirm that redirection will work on Windows (I wasn't able to after a quick search), then I'd be fine with this approach as well. | |
| 26 | ||
| libcxx/utils/data/unicode/README.txt | ||
| 2 | Also, can you please include a copy-pasteable command to update all of those files at once? Something like a curl invocation. This is helpful to quickly update those files without having to think too much. | |
Comment Actions
Thanks for the review comments!
| libcxx/utils/CMakeLists.txt | ||
|---|---|---|
| 2 | The generating script's output has been updated in D126971. | |
| 17–19 | I really dislike hard-coding the output path in this script. Neither do I have access to Windows to validate whether > works. Instead I will change the script, when it's called with one argument it will use that argument as filename to write to. Otherwise it will still write to stdout. | |
For developers and the CI a hard requirement is fine. But this might not be good for vendors.
We could make the target using formatting conditional on whether or not the binary is found.
That would however mean that if clang-format isn't found the CI will pass.
So I'm considering to make all these targets to depend on clang-format.
@ldionne Let's discuss what the best approach is.