[libcxx-commits] [PATCH] D107596: [libc++][doc] Improve contributor documentation.

[Mark de Wever via Phabricator via libcxx-commits]

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

I addressed the issues in rGd2bc4fa3c70ad9dd1723b2e75c9059c5db2052cf <https://reviews.llvm.org/rGd2bc4fa3c70ad9dd1723b2e75c9059c5db2052cf>.



================
Comment at: libcxx/docs/Contributing.rst:73-74
+
+For features in a not yet ratified C++ Standard libc++ makes no guarantees
+about the implementation status nor the ABI stability. After the C++ Standard
+is ratified libc++ promises a conforming ABI stable implementation. When
----------------
Quuxplusone wrote:
> Mordante wrote:
> > ldionne wrote:
> > > 
> > I only change `a C++ Standard` to `the a C++ Standard`. In the view of ISO there's only one standard.
> There //is// only one Standard, but there //have been// many Standards over the past few years. If we're going to split hairs, there's no such thing as "a Standard that has not yet been ratified," because before it's been ratified it's not a Standard yet — merely a draft. ;)
> 
> Anyway, I'd take Louis's wording with one minor tweak to the grammatical placement of `yet`. Also I'm ambivalent as to whether the indicated word should be `or` or `nor`:
> ```
> Libc++ makes no guarantees about the implementation status [n]or the ABI stability
> of features that have not yet been ratified in a C++ Standard. After the C++ Standard
> is ratified, libc++ promises a conforming and ABI-stable implementation.
> ```
I still like `the` better, so I keep that.
I have a slight preference for the `or` over the `nor`.
I moved the `yet` as you suggested.


================
Comment at: libcxx/docs/Contributing.rst:101
+
+* ``include/foo`` the contents of the file should be guarded in an ``ifdef``
+  and always include ``<version>``
----------------
Quuxplusone wrote:
> ```
> ``libcxx/include/foo``: The file should include ``<version>`` and then guard the feature with an ``ifdef``.
> ```
> Throughout, I recommend giving full paths. You currently say `libcxx/include/__config_site.in` (full path) but only `include/foo` (partial path); which means I'm not sure whether `CMakeLists.txt` on line 89 is a full path or not.
> I also recommend using punctuation: `somefile: Do something.`, not `somefile do something`.
Seems `libcxx/include/__config_site.in` is the only full path, the others are all relative. I changed them all to full paths.


================
Comment at: libcxx/docs/Contributing.rst:109
+
+    // Make sure all feature tests macros are always available.
+    #include <version>
----------------
Quuxplusone wrote:
> `// Make sure all feature-test macros are available.`
Since this document describes the current implementation, I applied this change here and in `<format>` and `<ranges>`.


================
Comment at: libcxx/docs/Contributing.rst:111
+    #include <version>
+    // Only enable the contents of the header when libc++ was build with LIBCXX_ENABLE_INCOMPLETE_FEATURES enabled
+    #if !defined(_LIBCPP_HAS_NO_INCOMPLETE_FOO)
----------------
Quuxplusone wrote:
> `// Enable the contents of the header only when libc++ was built with LIBCXX_ENABLE_INCOMPLETE_FEATURES.`
Since this document describes the current implementation, I applied this change here and in `<format>` and `<ranges>`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107596