[libcxx-commits] [PATCH] D129668: [libc++] Improve updating data files.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

I really like this except for the `clang-format` requirement -- thanks!



================
Comment at: libcxx/utils/CMakeLists.txt:2
 
+find_program(LIBCXX_CLANG_FORMAT "clang-format" REQUIRED)
+
----------------
Mordante wrote:
> 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.
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?


================
Comment at: libcxx/utils/CMakeLists.txt:16
 
+add_custom_target(libcxx-generate-extened-grapheme-cluster-tables
+    COMMAND
----------------



================
Comment at: libcxx/utils/CMakeLists.txt:19-21
+        "${LIBCXX_SOURCE_DIR}/utils/generate_extended_grapheme_cluster_table.py"
+        ">"
+        "${LIBCXX_SOURCE_DIR}/include/__format/extended_grapheme_cluster_table.h"
----------------
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.


================
Comment at: libcxx/utils/CMakeLists.txt:28
+
+add_custom_target(libcxx-generate-extened-grapheme-cluster-tests
+    COMMAND
----------------



================
Comment at: libcxx/utils/data/unicode/README.txt:1
+Contains various Unicode data files
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129668