[libcxx-commits] [PATCH] D101303: [libc++][CI] Fix check-generated-output.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante marked an inline comment as done.
Mordante added a comment.

Thanks for the reviews.



================
Comment at: libcxx/utils/ci/run-buildbot:146-156
     # Reject patches that introduce non-ASCII characters or hard tabs.
-    ! grep -rn '[^ -~]' libcxx/include/
+    ! grep -rn '[^ -~]' libcxx/include/ || false
     # Reject patches that don't update the generated output correctly.
     python3 libcxx/utils/generate_feature_test_macro_components.py
     python3 libcxx/utils/generate_header_inclusion_tests.py
     python3 libcxx/utils/generate_header_tests.py
     git diff | tee ${BUILD_DIR}/generated_output.patch
----------------
Quuxplusone wrote:
> I think I actually liked your original reordering; it puts the two `|| false`s close together visually. Meanwhile, the comment can easily be 1 line instead of 7.
> ```
>     ~~~
>     # Check if the diffs are empty, fail otherwise.
>     # "|| false" works around https://stackoverflow.com/questions/57681955/set-e-does-not-respect-logical-not
>     ! grep -q '^--- a' ${BUILD_DIR}/generated_output.patch || false
>     # Reject patches that introduce non-ASCII characters or hard tabs.
>     ! grep -rn '[^ -~]' libcxx/include/ || false
>     # Check that no dependency cycles have been introduced.
>     python3 libcxx/utils/graph_header_deps.py >/dev/null
> ```
> WDYT?
I reordered the code again.
I don't mind shortening the comment a bit, but I feel your version is too terse.
I changed it to two lines, including the stackoverflow link.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101303