[PATCH] D133249: [libc++] Documents details of the pre-commit CI.

[Nikolas Klauser via Phabricator via cfe-commits]

philnik accepted this revision as: philnik.
philnik added a comment.

Mostly LGTM. Just a few nits.



================
Comment at: clang/www/hacking.html:276
+  <!--=====================================================================-->
+  <h3 id="testingLibcxx">Testing changes affecting libcxx</h3>
+  <!--=====================================================================-->
----------------
To make it consistent throughout


================
Comment at: clang/www/hacking.html:284
+      <li>Changing compiler builtins, especially the builtins used for type traits
+      or replacements of library functions like <tt>std::move</tt>,
+      <tt>std::forward</tt>.</li>
----------------



================
Comment at: clang/www/hacking.html:296
+  file will also add the libc++ group to the list of reviewers. The status of
+  the build will be available in Phabricator.</p>
+
----------------



================
Comment at: clang/www/hacking.html:300
+  CI lacks the resources to build every Clang diff. A single run takes about
+  one hour. Secondly most changes of Clang won't affect libc++.</a>
+
----------------
Having a secondly without a firstly seems weird.


================
Comment at: clang/www/hacking.html:310
+
+  <p>Unlike Clang, libc++ supports multiple versions of Clang. Therefore when a
+  patch changes the diagnostics it might be required to use a regex in the
----------------
This reads like you want to say that clang doesn't support multiple versions of clang, which seems self-evident. Maybe just drop the `Unlike Clang, `?


================
Comment at: libcxx/docs/Contributing.rst:87
 
-* C++20 for the Linux platform.
-* MacOS C++20 for the Apple platform.
+* C++XX for the Linux platform (where XX is the latest C++ version).
+* MacOS X86_64 and MacOS arm64 for the Apple platform.
----------------
Complete nit, but `XX` reads to me like the digits have to be the same. I'd suggest `XY` to make it obvious that they can be different.


================
Comment at: libcxx/docs/Contributing.rst:88
+* C++XX for the Linux platform (where XX is the latest C++ version).
+* MacOS X86_64 and MacOS arm64 for the Apple platform.
+
----------------



================
Comment at: libcxx/docs/Contributing.rst:107
+
+The CI tests libc++ for all :ref:`supported platforms <SupportedPlatforms>`.
+The build is started for every diff uploaded. A complete CI run takes
----------------
Not really currently. We still claim FreeBSD support without a CI runner.


================
Comment at: libcxx/docs/Contributing.rst:231-233
+  export CC=clang-14
+  export CXX=clang++-14
+  run-buildbot generic-cxx17
----------------
Complete nit, but I think `CC=clang-14 CXX=clang++-14 run-builtbot generic-cxx17` as an example would be better to avoid polluting people's environment if they're unfamiliar with a terminal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133249