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

[Mark de Wever via Phabricator via libcxx-commits]

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

Thanks for the review comments!



================
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"
----------------
ldionne wrote:
> 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.
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`.


================
Comment at: libcxx/utils/CMakeLists.txt:2
 
+find_program(LIBCXX_CLANG_FORMAT "clang-format" REQUIRED)
+
----------------
ldionne wrote:
> 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?
The generating script's output has been updated in D126971.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129668