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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
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
----------------
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.
```


================
Comment at: libcxx/docs/Contributing.rst:101
+
+* ``include/foo`` the contents of the file should be guarded in an ``ifdef``
+  and always include ``<version>``
----------------
```
``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`.


================
Comment at: libcxx/docs/Contributing.rst:109
+
+    // Make sure all feature tests macros are always available.
+    #include <version>
----------------
`// Make sure all feature-test macros are available.`


================
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)
----------------
`// Enable the contents of the header only when libc++ was built with LIBCXX_ENABLE_INCOMPLETE_FEATURES.`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107596