[libcxx-commits] [libcxx] [libc++] Add coding guidelines to the docs (PR #117051)
Nikolas Klauser via libcxx-commits
libcxx-commits at lists.llvm.org
Sat Nov 23 03:17:08 PST 2024
https://github.com/philnik777 updated https://github.com/llvm/llvm-project/pull/117051
>From 86cf692cce739c89c02b00f3e4e37ab306ee0cc4 Mon Sep 17 00:00:00 2001
From: Nikolas Klauser <nikolasklauser at berlin.de>
Date: Wed, 20 Nov 2024 22:13:45 +0100
Subject: [PATCH] [libc++] Add coding guidelines to the docs
---
libcxx/docs/CodingGuidelines.rst | 172 ++++++++++++++++++
libcxx/docs/Contributing.rst | 63 ++-----
.../docs/DesignDocs/ExtendedCXX03Support.rst | 69 -------
libcxx/docs/DesignDocs/NodiscardPolicy.rst | 42 -----
libcxx/docs/index.rst | 2 +-
5 files changed, 187 insertions(+), 161 deletions(-)
create mode 100644 libcxx/docs/CodingGuidelines.rst
delete mode 100644 libcxx/docs/DesignDocs/NodiscardPolicy.rst
diff --git a/libcxx/docs/CodingGuidelines.rst b/libcxx/docs/CodingGuidelines.rst
new file mode 100644
index 00000000000000..c1369ca8ea38c3
--- /dev/null
+++ b/libcxx/docs/CodingGuidelines.rst
@@ -0,0 +1,172 @@
+.. _CodingGuidelines:
+
+========================
+libc++ Coding Guidelines
+========================
+
+.. contents::
+ :local:
+
+Use ``__ugly_names`` for implementation details
+===============================================
+
+Libc++ uses ``__ugly_names`` or ``_UglyNames`` for implementation details. These names are reserved for implementations,
+so users may not use them in their own applications. When using a name like ``T``, a user may have defined a macro that
+changes the meaning of ``T``. By using ``__ugly_names`` we avoid that problem.
+
+This is partially enforced by the clang-tidy check ``readability-identifier-naming`` and
+``libcxx/test/libcxx/system_reserved_names.gen.py``.
+
+Don't use argument-dependent lookup unless required by the standard
+===================================================================
+
+Unqualified function calls are susceptible to
+`argument-dependent lookup (ADL) <https://en.cppreference.com/w/cpp/language/adl>`_. This means calling
+``move(UserType)`` might not call ``std::move``. Therefore, function calls must use qualified names to avoid ADL. Some
+functions in the standard library `require ADL usage <http://eel.is/c++draft/contents#3>`_. Names of classes, variables,
+concepts, and type aliases are not subject to ADL. They don't need to be qualified.
+
+Function overloading also applies to operators. Using ``&user_object`` may call a user-defined ``operator&``. Use
+``std::addressof`` instead. Similarly, to avoid invoking a user-defined ``operator,``, make sure to cast the result to
+``void`` when using the ``,`` or avoid it in the first place. For example:
+
+.. code-block:: cpp
+
+ for (; __first1 != __last1; ++__first1, (void)++__first2) {
+ ...
+ }
+
+This is mostly enforced by the clang-tidy checks ``libcpp-robust-against-adl`` and ``libcpp-qualify-declval``.
+
+Avoid including public headers
+==============================
+
+libc++ uses implementation-detail headers for most code. These are in a directory that starts with two underscores
+(e.g. ``<__type_traits/decay.h>``). These detail headers are significantly smaller than their public counterparts.
+This reduces the amount of code that is included in a single public header, reducing compile times in turn.
+
+Add ``_LIBCPP_HIDE_FROM_ABI`` unless you know better
+====================================================
+
+``_LIBCPP_HIDE_FROM_ABI`` should be on every function in the library unless there is a reason not to do so. The main
+reason to not add ``_LIBCPP_HIDE_FROM_ABI`` is if a function is exported from the libc++ built library. In that case a
+function should be marked ``_LIBCPP_EXPORTED_FROM_ABI``. Virtual functions should be marked
+``_LIBCPP_HIDE_FROM_ABI_VIRTUAL`` instead.
+
+This is mostly enforced by the clang-tidy checks ``libcpp-hide-from-abi`` and ``libcpp-avoid-abi-tag-on-virtual``.
+
+Always define macros
+====================
+
+Macros should usually be defined in all configurations, instead of defining them when they're enabled and leaving them
+undefined otherwise. For example, use
+
+.. code-block:: cpp
+
+ #if SOMETHING
+ # define _LIBCPP_SOMETHING_ENABLED 1
+ #else
+ # define _LIBCPP_SOMETHING_ENABLED 0
+ #endif
+
+instead of
+
+.. code-block:: cpp
+
+ #if SOMETHING
+ # define _LIBCPP_SOMETHING_ENABLED
+ #endif
+
+This makes it significantly easier to catch missing includes, since Clang and GCC will warn when using and undefined
+marco inside an ``#if`` statement when using ``-Wundef``. Some macros in libc++ don't use this style yet, so this only
+applies when introducing a new macro.
+
+This is partially enforced by the clang-tidy check ``libcpp-internal-ftms``.
+
+Use ``_LIBCPP_STD_VER``
+=======================
+
+libc++ defines the macro ``_LIBCPP_STD_VER`` for the different libc++ dialects. This should be used instead of
+``__cplusplus``. The form ``_LIBCPP_STD_VER >= <version>`` is preferred over ``_LIBCPP_STD_VER > <previous-version>``.
+
+This is mostly enforced by the clang-tidy check ``libcpp-cpp-version-check``.
+
+Use \_\_ugly\_\_ spellings of vendor attributes
+===============================================
+
+Vendor attributes should always be \_\_uglified\_\_ to avoid naming clashes with user-defined macros. For gnu-style
+attributes this takes the form ``__attribute__((__attribute__))``. C++11-style attributes look like
+``[[_Clang::__attribute__]]`` or ``[[__gnu__::__attribute__]]`` for Clang or GCC attributes respectively. Clang and GCC
+also support standard attributes in earlier language dialects than they were introduced. These should be spelled as
+``[[__attribute__]]``. MSVC currently doesn't provide alternative spellings for their attributes, so these should be
+avoided if at all possible.
+
+This is enforced by the clang-tidy check ``libcpp-uglify-attributes``.
+
+Use C++11 extensions in C++03 code if they simplify the code
+============================================================
+
+libc++ only supports Clang in C++98/03 mode. Clang provides many C++11 features in C++03, making it possible to write a
+lot of code in a simpler way than if we were restricted to C++03 features. Some use of extensions is even mandatory,
+since libc++ supports move semantics in C++03.
+
+Use ``using`` aliases instead of ``typedef``
+============================================
+
+``using`` aliases are generally easier to read and support templates. Some code in libc++ uses ``typedef`` for
+historical reasons.
+
+Write SFINAE with ``requires`` clauses in C++20-only code
+=========================================================
+
+``requires`` clauses can be significantly easier to read than ``enable_if`` and friends in some cases, since concepts
+subsume other concepts. This means that overloads based on traits can be written without negating more general cases.
+They also show intent better.
+
+Write ``enable_if`` as ``enable_if_t<conditon, int> = 0``
+=========================================================
+
+The form ``enable_if_t<condition, int> = 0`` is the only one that works in every language mode and for overload sets
+using the same template arguments otherwise. If the code must work in C++11 or C++03, the libc++-internal alias
+``__enable_if_t`` can be used instead.
+
+Prefer alias templates over class templates
+===========================================
+
+Alias templates are much more light weight than class templates, since they don't require new instantiations for
+different types. If the only member of a class is an alias, like in type traits, alias templates should be used if
+possible. They do force more eager evaluation though, which can be a problem in some cases.
+
+Apply ``[[nodiscard]]`` where relevant
+======================================
+
+Libc++ adds ``[[nodiscard]]`` whenever relevant to catch potential bugs. The standards committee has decided to not have
+a recommended practice where to put them, so libc++ applies it whenever it makes sense to catch potential bugs.
+
+``[[nodiscard]]`` should be applied to functions
+
+- where discarding the return value is most likely a correctness issue. For example a locking constructor in
+ ``unique_lock``.
+
+- where discarding the return value likely points to the user wanting to do something different. For example
+ ``vector::empty()``, which probably should have been ``vector::clear()``.
+
+ This can help spotting bugs easily which otherwise may take a very long time to find.
+
+- which return a constant. For example ``numeric_limits::min()``.
+- which only observe a value. For example ``string::size()``.
+
+ Code that discards values from these kinds of functions is dead code. It can either be removed, or the programmer
+ meant to do something different.
+
+- where discarding the value is most likely a misuse of the function. For example ``std::find(first, last, val)``.
+
+ This protects programmers from assuming too much about how the internals of a function work, making code more robust
+ in the presence of future optimizations.
+
+What should be done when adding ``[[nodiscard]]`` to a function?
+----------------------------------------------------------------
+
+Applications of ``[[nodiscard]]`` are code like any other code, so we aim to test them on public interfaces. This can be
+done with a ``.verify.cpp`` test. Many examples are available. Just look for tests with the suffix
+``.nodiscard.verify.cpp``.
diff --git a/libcxx/docs/Contributing.rst b/libcxx/docs/Contributing.rst
index b15fc88943d59b..5bcd6e3a7f03e7 100644
--- a/libcxx/docs/Contributing.rst
+++ b/libcxx/docs/Contributing.rst
@@ -36,56 +36,10 @@ Every change in libc++ must come with appropriate tests. Libc++ has an extensive
should be run locally by developers before submitting patches and is also run as part of our CI
infrastructure. The documentation about writing tests and running them is :ref:`here <testing>`.
-Coding standards
-================
+Coding Guidelines
+=================
-In general, libc++ follows the `LLVM Coding Standards <https://llvm.org/docs/CodingStandards.html>`_.
-There are some deviations from these standards.
-
-Libc++ uses ``__ugly_names``. These names are reserved for implementations, so
-users may not use them in their own applications. When using a name like ``T``,
-a user may have defined a macro that changes the meaning of ``T``. By using
-``__ugly_names`` we avoid that problem. Other standard libraries and compilers
-use these names too. To avoid common clashes with other uglified names used in
-other implementations (e.g. system headers), the test in
-``libcxx/test/libcxx/system_reserved_names.gen.py`` contains the list of
-reserved names that can't be used.
-
-Unqualified function calls are susceptible to
-`argument-dependent lookup (ADL) <https://en.cppreference.com/w/cpp/language/adl>`_.
-This means calling ``move(UserType)`` might not call ``std::move``. Therefore,
-function calls must use qualified names to avoid ADL. Some functions in the
-standard library `require ADL usage <http://eel.is/c++draft/contents#3>`_.
-Names of classes, variables, concepts, and type aliases are not subject to ADL.
-They don't need to be qualified.
-
-Function overloading also applies to operators. Using ``&user_object`` may call
-a user-defined ``operator&``. Use ``std::addressof`` instead. Similarly, to
-avoid invoking a user-defined ``operator,``, make sure to cast the result to
-``void`` when using the ``,``. For example:
-
-.. code-block:: cpp
-
- for (; __first1 != __last1; ++__first1, (void)++__first2) {
- ...
- }
-
-In general, try to follow the style of existing code. There are a few
-exceptions:
-
-- Prefer ``using foo = int`` over ``typedef int foo``. The compilers supported
- by libc++ accept alias declarations in all standard modes.
-
-Other tips are:
-
-- Keep the number of formatting changes in patches minimal.
-- Provide separate patches for style fixes and for bug fixes or features. Keep in
- mind that large formatting patches may cause merge conflicts with other patches
- under review. In general, we prefer to avoid large reformatting patches.
-- Keep patches self-contained. Large and/or complicated patches are harder to
- review and take a significant amount of time. It's fine to have multiple
- patches to implement one feature if the feature can be split into
- self-contained sub-tasks.
+libc++'s coding guidelines are documented :ref:`here <CodingGuidelines>`.
Resources
@@ -186,6 +140,17 @@ rule -- for very simple patches, use your judgement. The `"libc++" review group
consists of frequent libc++ contributors with a good understanding of the project's
guidelines -- if you would like to be added to it, please reach out on Discord.
+Some tips:
+
+- Keep the number of formatting changes in patches minimal.
+- Provide separate patches for style fixes and for bug fixes or features. Keep in
+ mind that large formatting patches may cause merge conflicts with other patches
+ under review. In general, we prefer to avoid large reformatting patches.
+- Keep patches self-contained. Large and/or complicated patches are harder to
+ review and take a significant amount of time. It's fine to have multiple
+ patches to implement one feature if the feature can be split into
+ self-contained sub-tasks.
+
Exporting new symbols from the library
======================================
diff --git a/libcxx/docs/DesignDocs/ExtendedCXX03Support.rst b/libcxx/docs/DesignDocs/ExtendedCXX03Support.rst
index 8c18e563e81997..42d59db2bb7a41 100644
--- a/libcxx/docs/DesignDocs/ExtendedCXX03Support.rst
+++ b/libcxx/docs/DesignDocs/ExtendedCXX03Support.rst
@@ -47,72 +47,3 @@ Provided C++11 Library Extensions
This section will be updated once the libc++ developer community has further discussed the
future of C++03 with libc++.
-
-
-Using Minimal C++11 in libc++
-=============================
-
-This section is for developers submitting patches to libc++. It describes idioms that should be
-used in libc++ code, even in C++03, and the reasons behind them.
-
-
-Use Alias Templates over Class Templates
-----------------------------------------
-
-Alias templates should be used instead of class templates in metaprogramming. Unlike class templates,
-Alias templates do not produce a new instantiation every time they are used. This significantly
-decreases the amount of memory used by the compiler.
-
-For example, libc++ should not use ``add_const`` internally. Instead it should use an alias template
-like
-
-.. code-block:: cpp
-
- template <class _Tp>
- using _AddConst = const _Tp;
-
-Use Default Template Parameters for SFINAE
-------------------------------------------
-
-There are three places in a function declaration that SFINAE may occur: In the template parameter list,
-in the function parameter list, and in the return type. For example:
-
-.. code-block:: cpp
-
- template <class _Tp, class _ = enable_if_t</*...*/ >
- void foo(_Tp); // #1
-
- template <class _Tp>
- void bar(_Tp, enable_if_t</*...*/>* = nullptr); // # 2
-
- template <class _Tp>
- enable_if_t</*...*/> baz(_Tp); // # 3
-
-Using default template parameters for SFINAE (#1) should always be preferred.
-
-Option #2 has two problems. First, users can observe and accidentally pass values to the SFINAE
-function argument. Second, the default argument creates a live variable, which causes debug
-information to be emitted containing the text of the SFINAE.
-
-Option #3 can also cause more debug information to be emitted than is needed, because the function
-return type will appear in the debug information.
-
-Use ``unique_ptr`` when allocating memory
-------------------------------------------
-
-The standard library often needs to allocate memory and then construct a user type in it.
-If the users constructor throws, the library needs to deallocate that memory. The idiomatic way to
-achieve this is with ``unique_ptr``.
-
-``__builtin_new_allocator`` is an example of this idiom. Example usage would look like:
-
-.. code-block:: cpp
-
- template <class T>
- T* __create() {
- using _UniquePtr = unique_ptr<void*, __default_new_allocator::__default_new_deleter>;
- _UniquePtr __p = __default_new_allocator::__allocate_bytes(sizeof(T), alignof(T));
- T* __res = ::new(__p.get()) T();
- (void)__p.release();
- return __res;
- }
diff --git a/libcxx/docs/DesignDocs/NodiscardPolicy.rst b/libcxx/docs/DesignDocs/NodiscardPolicy.rst
deleted file mode 100644
index afbb18b0096d73..00000000000000
--- a/libcxx/docs/DesignDocs/NodiscardPolicy.rst
+++ /dev/null
@@ -1,42 +0,0 @@
-===================================================
-Guidelines for applying ``[[nodiscard]]`` in libc++
-===================================================
-
-Libc++ adds ``[[nodiscard]]`` to functions in a lot of places. The standards
-committee has decided to not have a recommended practice where to put them, so
-this document lists where ``[[nodiscard]]`` should be applied in libc++.
-
-When should ``[[nodiscard]]`` be added to functions?
-====================================================
-
-``[[nodiscard]]`` should be applied to functions
-
-- where discarding the return value is most likely a correctness issue.
- For example a locking constructor in ``unique_lock``.
-
-- where discarding the return value likely points to the user wanting to do
- something different. For example ``vector::empty()``, which probably should
- have been ``vector::clear()``.
-
- This can help spotting bugs easily which otherwise may take a very long time
- to find.
-
-- which return a constant. For example ``numeric_limits::min()``.
-- which only observe a value. For example ``string::size()``.
-
- Code that discards values from these kinds of functions is dead code. It can
- either be removed, or the programmer meant to do something different.
-
-- where discarding the value is most likely a misuse of the function. For
- example ``find``.
-
- This protects programmers from assuming too much about how the internals of
- a function work, making code more robust in the presence of future
- optimizations.
-
-What should be done when adding ``[[nodiscard]]`` to a function?
-================================================================
-
-Applications of ``[[nodiscard]]`` are code like any other code, so we aim to
-test them. This can be done with a ``.verify.cpp`` test. Many examples are
-available. Just look for tests with the suffix ``.nodiscard.verify.cpp``.
diff --git a/libcxx/docs/index.rst b/libcxx/docs/index.rst
index a9610cbb4db3a4..37912e9f8d03e6 100644
--- a/libcxx/docs/index.rst
+++ b/libcxx/docs/index.rst
@@ -38,6 +38,7 @@ Getting Started with libc++
UserDocumentation
VendorDocumentation
Contributing
+ CodingGuidelines
TestingLibcxx
ImplementationDefinedBehavior
Modules
@@ -216,7 +217,6 @@ Design Documents
DesignDocs/FeatureTestMacros
DesignDocs/FileTimeType
DesignDocs/HeaderRemovalPolicy
- DesignDocs/NodiscardPolicy
DesignDocs/NoexceptPolicy
DesignDocs/PSTLIntegration
DesignDocs/ThreadingSupportAPI
More information about the libcxx-commits
mailing list