Skip to content

[clang-format][CMake] Generate formatting options docs during build #113739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

boomanaiden154
Copy link
Contributor

This patch changes up the CMake configuration so that ClangFormatStyleOptions.rst has the format style options section generated by the dump_format_style.py python script during the build rather than manually. This allows us to remove ~6000 lines of automatically generated documentation from the repository.

If the context of the options is needed while generating the docs, it is easy enough to open up the generated file with the options from within the build directory after the clang-format-style-options target has been built.

This patch changes up the CMake configuration so that
ClangFormatStyleOptions.rst has the format style options section
generated by the dump_format_style.py python script during the build
rather than manually. This allows us to remove ~6000 lines of
automatically generated documentation from the repository.

If the context of the options is needed while generating the docs, it is
easy enough to open up the generated file with the options from within
the build directory after the clang-format-style-options target has been
built.
@llvmbot llvmbot added the clang Clang issues not falling into any other category label Oct 25, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 25, 2024

@llvm/pr-subscribers-clang-format

@llvm/pr-subscribers-clang

Author: Aiden Grossman (boomanaiden154)

Changes

This patch changes up the CMake configuration so that ClangFormatStyleOptions.rst has the format style options section generated by the dump_format_style.py python script during the build rather than manually. This allows us to remove ~6000 lines of automatically generated documentation from the repository.

If the context of the options is needed while generating the docs, it is easy enough to open up the generated file with the options from within the build directory after the clang-format-style-options target has been built.


Patch is 181.72 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/113739.diff

3 Files Affected:

  • (modified) clang/docs/CMakeLists.txt (+10-1)
  • (modified) clang/docs/ClangFormatStyleOptions.rst (+2-6649)
  • (modified) clang/docs/tools/dump_format_style.py (+1-1)
diff --git a/clang/docs/CMakeLists.txt b/clang/docs/CMakeLists.txt
index 4fecc007f59954..313b0efd1dbd64 100644
--- a/clang/docs/CMakeLists.txt
+++ b/clang/docs/CMakeLists.txt
@@ -143,8 +143,17 @@ if (LLVM_ENABLE_SPHINX)
     gen_rst_file_from_td(DiagnosticsReference.rst -gen-diag-docs ../include/clang/Basic/Diagnostic.td "${docs_targets}")
     gen_rst_file_from_td(ClangCommandLineReference.rst -gen-opt-docs ../include/clang/Driver/ClangOptionDocs.td "${docs_targets}")
 
+    add_custom_target(clang-format-style-options
+      COMMAND "${Python3_EXECUTABLE}" "${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+      WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
+      DEPENDS "${CLANG_SOURCE_DIR}/include/clang/Format/Format.h"
+              "${CLANG_SOURCE_DIR}/include/clang/Tooling/Inclusions/IncludeStyle.h"
+              "${CLANG_SOURCE_DIR}/docs/tools/dump_format_style.py"
+              copy-clang-rst-docs
+    )
+
     foreach(target ${docs_targets})
-      add_dependencies(${target} copy-clang-rst-docs)
+      add_dependencies(${target} copy-clang-rst-docs clang-format-style-options)
     endforeach()
   endif()
 endif()
diff --git a/clang/docs/ClangFormatStyleOptions.rst b/clang/docs/ClangFormatStyleOptions.rst
index 9ef1fd5f36d1d3..508a058de6f768 100644
--- a/clang/docs/ClangFormatStyleOptions.rst
+++ b/clang/docs/ClangFormatStyleOptions.rst
@@ -1,9 +1,3 @@
-..
-  !!!!NOTE!!!!
-  This file is automatically generated, in part. Do not edit the style options
-  in this file directly. Instead, modify them in include/clang/Format/Format.h
-  and run the docs/tools/dump_format_style.py script to update this file.
-
 .. raw:: html
 
       <style type="text/css">
@@ -185,6651 +179,10 @@ the configuration (without a prefix: ``Auto``).
     subdirectories. This is also possible through the command line, e.g.:
     ``--style={BasedOnStyle: InheritParentConfig, ColumnLimit: 20}``
 
+.. This section of the file is automatically generated by the
+  dump_format_style.py tool and should be updated manually.
 .. START_FORMAT_STYLE_OPTIONS
 
-.. _AccessModifierOffset:
-
-**AccessModifierOffset** (``Integer``) :versionbadge:`clang-format 3.3` :ref:`¶ <AccessModifierOffset>`
-  The extra indent or outdent of access modifiers, e.g. ``public:``.
-
-.. _AlignAfterOpenBracket:
-
-**AlignAfterOpenBracket** (``BracketAlignmentStyle``) :versionbadge:`clang-format 3.8` :ref:`¶ <AlignAfterOpenBracket>`
-  If ``true``, horizontally aligns arguments after an open bracket.
-
-  This applies to round brackets (parentheses), angle brackets and square
-  brackets.
-
-  Possible values:
-
-  * ``BAS_Align`` (in configuration: ``Align``)
-    Align parameters on the open bracket, e.g.:
-
-    .. code-block:: c++
-
-      someLongFunction(argument1,
-                       argument2);
-
-  * ``BAS_DontAlign`` (in configuration: ``DontAlign``)
-    Don't align, instead use ``ContinuationIndentWidth``, e.g.:
-
-    .. code-block:: c++
-
-      someLongFunction(argument1,
-          argument2);
-
-  * ``BAS_AlwaysBreak`` (in configuration: ``AlwaysBreak``)
-    Always break after an open bracket, if the parameters don't fit
-    on a single line, e.g.:
-
-    .. code-block:: c++
-
-      someLongFunction(
-          argument1, argument2);
-
-  * ``BAS_BlockIndent`` (in configuration: ``BlockIndent``)
-    Always break after an open bracket, if the parameters don't fit
-    on a single line. Closing brackets will be placed on a new line.
-    E.g.:
-
-    .. code-block:: c++
-
-      someLongFunction(
-          argument1, argument2
-      )
-
-
-    .. note::
-
-     This currently only applies to braced initializer lists (when
-     ``Cpp11BracedListStyle`` is ``true``) and parentheses.
-
-
-
-.. _AlignArrayOfStructures:
-
-**AlignArrayOfStructures** (``ArrayInitializerAlignmentStyle``) :versionbadge:`clang-format 13` :ref:`¶ <AlignArrayOfStructures>`
-  If not ``None``, when using initialization for an array of structs
-  aligns the fields into columns.
-
-
-  .. note::
-
-   As of clang-format 15 this option only applied to arrays with equal
-   number of columns per row.
-
-  Possible values:
-
-  * ``AIAS_Left`` (in configuration: ``Left``)
-    Align array column and left justify the columns e.g.:
-
-    .. code-block:: c++
-
-      struct test demo[] =
-      {
-          {56, 23,    "hello"},
-          {-1, 93463, "world"},
-          {7,  5,     "!!"   }
-      };
-
-  * ``AIAS_Right`` (in configuration: ``Right``)
-    Align array column and right justify the columns e.g.:
-
-    .. code-block:: c++
-
-      struct test demo[] =
-      {
-          {56,    23, "hello"},
-          {-1, 93463, "world"},
-          { 7,     5,    "!!"}
-      };
-
-  * ``AIAS_None`` (in configuration: ``None``)
-    Don't align array initializer columns.
-
-
-
-.. _AlignConsecutiveAssignments:
-
-**AlignConsecutiveAssignments** (``AlignConsecutiveStyle``) :versionbadge:`clang-format 3.8` :ref:`¶ <AlignConsecutiveAssignments>`
-  Style of aligning consecutive assignments.
-
-  ``Consecutive`` will result in formattings like:
-
-  .. code-block:: c++
-
-    int a            = 1;
-    int somelongname = 2;
-    double c         = 3;
-
-  Nested configuration flags:
-
-  Alignment options.
-
-  They can also be read as a whole for compatibility. The choices are:
-
-  * ``None``
-  * ``Consecutive``
-  * ``AcrossEmptyLines``
-  * ``AcrossComments``
-  * ``AcrossEmptyLinesAndComments``
-
-  For example, to align across empty lines and not across comments, either
-  of these work.
-
-  .. code-block:: c++
-
-    AlignConsecutiveAssignments: AcrossEmptyLines
-
-    AlignConsecutiveAssignments:
-      Enabled: true
-      AcrossEmptyLines: true
-      AcrossComments: false
-
-  * ``bool Enabled`` Whether aligning is enabled.
-
-    .. code-block:: c++
-
-      #define SHORT_NAME       42
-      #define LONGER_NAME      0x007f
-      #define EVEN_LONGER_NAME (2)
-      #define foo(x)           (x * x)
-      #define bar(y, z)        (y + z)
-
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int aaaa : 1;
-      int b    : 12;
-      int ccc  : 8;
-
-      int         aaaa = 12;
-      float       b = 23;
-      std::string ccc;
-
-  * ``bool AcrossEmptyLines`` Whether to align across empty lines.
-
-    .. code-block:: c++
-
-      true:
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int d            = 3;
-
-      false:
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int d = 3;
-
-  * ``bool AcrossComments`` Whether to align across comments.
-
-    .. code-block:: c++
-
-      true:
-      int d    = 3;
-      /* A comment. */
-      double e = 4;
-
-      false:
-      int d = 3;
-      /* A comment. */
-      double e = 4;
-
-  * ``bool AlignCompound`` Only for ``AlignConsecutiveAssignments``.  Whether compound assignments
-    like ``+=`` are aligned along with ``=``.
-
-    .. code-block:: c++
-
-      true:
-      a   &= 2;
-      bbb  = 2;
-
-      false:
-      a &= 2;
-      bbb = 2;
-
-  * ``bool AlignFunctionDeclarations`` Only for ``AlignConsecutiveDeclarations``. Whether function declarations
-    are aligned.
-
-    .. code-block:: c++
-
-      true:
-      unsigned int f1(void);
-      void         f2(void);
-      size_t       f3(void);
-
-      false:
-      unsigned int f1(void);
-      void f2(void);
-      size_t f3(void);
-
-  * ``bool AlignFunctionPointers`` Only for ``AlignConsecutiveDeclarations``. Whether function pointers are
-    aligned.
-
-    .. code-block:: c++
-
-      true:
-      unsigned i;
-      int     &r;
-      int     *p;
-      int      (*f)();
-
-      false:
-      unsigned i;
-      int     &r;
-      int     *p;
-      int (*f)();
-
-  * ``bool PadOperators`` Only for ``AlignConsecutiveAssignments``.  Whether short assignment
-    operators are left-padded to the same length as long ones in order to
-    put all assignment operators to the right of the left hand side.
-
-    .. code-block:: c++
-
-      true:
-      a   >>= 2;
-      bbb   = 2;
-
-      a     = 2;
-      bbb >>= 2;
-
-      false:
-      a >>= 2;
-      bbb = 2;
-
-      a     = 2;
-      bbb >>= 2;
-
-
-.. _AlignConsecutiveBitFields:
-
-**AlignConsecutiveBitFields** (``AlignConsecutiveStyle``) :versionbadge:`clang-format 11` :ref:`¶ <AlignConsecutiveBitFields>`
-  Style of aligning consecutive bit fields.
-
-  ``Consecutive`` will align the bitfield separators of consecutive lines.
-  This will result in formattings like:
-
-  .. code-block:: c++
-
-    int aaaa : 1;
-    int b    : 12;
-    int ccc  : 8;
-
-  Nested configuration flags:
-
-  Alignment options.
-
-  They can also be read as a whole for compatibility. The choices are:
-
-  * ``None``
-  * ``Consecutive``
-  * ``AcrossEmptyLines``
-  * ``AcrossComments``
-  * ``AcrossEmptyLinesAndComments``
-
-  For example, to align across empty lines and not across comments, either
-  of these work.
-
-  .. code-block:: c++
-
-    AlignConsecutiveBitFields: AcrossEmptyLines
-
-    AlignConsecutiveBitFields:
-      Enabled: true
-      AcrossEmptyLines: true
-      AcrossComments: false
-
-  * ``bool Enabled`` Whether aligning is enabled.
-
-    .. code-block:: c++
-
-      #define SHORT_NAME       42
-      #define LONGER_NAME      0x007f
-      #define EVEN_LONGER_NAME (2)
-      #define foo(x)           (x * x)
-      #define bar(y, z)        (y + z)
-
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int aaaa : 1;
-      int b    : 12;
-      int ccc  : 8;
-
-      int         aaaa = 12;
-      float       b = 23;
-      std::string ccc;
-
-  * ``bool AcrossEmptyLines`` Whether to align across empty lines.
-
-    .. code-block:: c++
-
-      true:
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int d            = 3;
-
-      false:
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int d = 3;
-
-  * ``bool AcrossComments`` Whether to align across comments.
-
-    .. code-block:: c++
-
-      true:
-      int d    = 3;
-      /* A comment. */
-      double e = 4;
-
-      false:
-      int d = 3;
-      /* A comment. */
-      double e = 4;
-
-  * ``bool AlignCompound`` Only for ``AlignConsecutiveAssignments``.  Whether compound assignments
-    like ``+=`` are aligned along with ``=``.
-
-    .. code-block:: c++
-
-      true:
-      a   &= 2;
-      bbb  = 2;
-
-      false:
-      a &= 2;
-      bbb = 2;
-
-  * ``bool AlignFunctionDeclarations`` Only for ``AlignConsecutiveDeclarations``. Whether function declarations
-    are aligned.
-
-    .. code-block:: c++
-
-      true:
-      unsigned int f1(void);
-      void         f2(void);
-      size_t       f3(void);
-
-      false:
-      unsigned int f1(void);
-      void f2(void);
-      size_t f3(void);
-
-  * ``bool AlignFunctionPointers`` Only for ``AlignConsecutiveDeclarations``. Whether function pointers are
-    aligned.
-
-    .. code-block:: c++
-
-      true:
-      unsigned i;
-      int     &r;
-      int     *p;
-      int      (*f)();
-
-      false:
-      unsigned i;
-      int     &r;
-      int     *p;
-      int (*f)();
-
-  * ``bool PadOperators`` Only for ``AlignConsecutiveAssignments``.  Whether short assignment
-    operators are left-padded to the same length as long ones in order to
-    put all assignment operators to the right of the left hand side.
-
-    .. code-block:: c++
-
-      true:
-      a   >>= 2;
-      bbb   = 2;
-
-      a     = 2;
-      bbb >>= 2;
-
-      false:
-      a >>= 2;
-      bbb = 2;
-
-      a     = 2;
-      bbb >>= 2;
-
-
-.. _AlignConsecutiveDeclarations:
-
-**AlignConsecutiveDeclarations** (``AlignConsecutiveStyle``) :versionbadge:`clang-format 3.8` :ref:`¶ <AlignConsecutiveDeclarations>`
-  Style of aligning consecutive declarations.
-
-  ``Consecutive`` will align the declaration names of consecutive lines.
-  This will result in formattings like:
-
-  .. code-block:: c++
-
-    int         aaaa = 12;
-    float       b = 23;
-    std::string ccc;
-
-  Nested configuration flags:
-
-  Alignment options.
-
-  They can also be read as a whole for compatibility. The choices are:
-
-  * ``None``
-  * ``Consecutive``
-  * ``AcrossEmptyLines``
-  * ``AcrossComments``
-  * ``AcrossEmptyLinesAndComments``
-
-  For example, to align across empty lines and not across comments, either
-  of these work.
-
-  .. code-block:: c++
-
-    AlignConsecutiveDeclarations: AcrossEmptyLines
-
-    AlignConsecutiveDeclarations:
-      Enabled: true
-      AcrossEmptyLines: true
-      AcrossComments: false
-
-  * ``bool Enabled`` Whether aligning is enabled.
-
-    .. code-block:: c++
-
-      #define SHORT_NAME       42
-      #define LONGER_NAME      0x007f
-      #define EVEN_LONGER_NAME (2)
-      #define foo(x)           (x * x)
-      #define bar(y, z)        (y + z)
-
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int aaaa : 1;
-      int b    : 12;
-      int ccc  : 8;
-
-      int         aaaa = 12;
-      float       b = 23;
-      std::string ccc;
-
-  * ``bool AcrossEmptyLines`` Whether to align across empty lines.
-
-    .. code-block:: c++
-
-      true:
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int d            = 3;
-
-      false:
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int d = 3;
-
-  * ``bool AcrossComments`` Whether to align across comments.
-
-    .. code-block:: c++
-
-      true:
-      int d    = 3;
-      /* A comment. */
-      double e = 4;
-
-      false:
-      int d = 3;
-      /* A comment. */
-      double e = 4;
-
-  * ``bool AlignCompound`` Only for ``AlignConsecutiveAssignments``.  Whether compound assignments
-    like ``+=`` are aligned along with ``=``.
-
-    .. code-block:: c++
-
-      true:
-      a   &= 2;
-      bbb  = 2;
-
-      false:
-      a &= 2;
-      bbb = 2;
-
-  * ``bool AlignFunctionDeclarations`` Only for ``AlignConsecutiveDeclarations``. Whether function declarations
-    are aligned.
-
-    .. code-block:: c++
-
-      true:
-      unsigned int f1(void);
-      void         f2(void);
-      size_t       f3(void);
-
-      false:
-      unsigned int f1(void);
-      void f2(void);
-      size_t f3(void);
-
-  * ``bool AlignFunctionPointers`` Only for ``AlignConsecutiveDeclarations``. Whether function pointers are
-    aligned.
-
-    .. code-block:: c++
-
-      true:
-      unsigned i;
-      int     &r;
-      int     *p;
-      int      (*f)();
-
-      false:
-      unsigned i;
-      int     &r;
-      int     *p;
-      int (*f)();
-
-  * ``bool PadOperators`` Only for ``AlignConsecutiveAssignments``.  Whether short assignment
-    operators are left-padded to the same length as long ones in order to
-    put all assignment operators to the right of the left hand side.
-
-    .. code-block:: c++
-
-      true:
-      a   >>= 2;
-      bbb   = 2;
-
-      a     = 2;
-      bbb >>= 2;
-
-      false:
-      a >>= 2;
-      bbb = 2;
-
-      a     = 2;
-      bbb >>= 2;
-
-
-.. _AlignConsecutiveMacros:
-
-**AlignConsecutiveMacros** (``AlignConsecutiveStyle``) :versionbadge:`clang-format 9` :ref:`¶ <AlignConsecutiveMacros>`
-  Style of aligning consecutive macro definitions.
-
-  ``Consecutive`` will result in formattings like:
-
-  .. code-block:: c++
-
-    #define SHORT_NAME       42
-    #define LONGER_NAME      0x007f
-    #define EVEN_LONGER_NAME (2)
-    #define foo(x)           (x * x)
-    #define bar(y, z)        (y + z)
-
-  Nested configuration flags:
-
-  Alignment options.
-
-  They can also be read as a whole for compatibility. The choices are:
-
-  * ``None``
-  * ``Consecutive``
-  * ``AcrossEmptyLines``
-  * ``AcrossComments``
-  * ``AcrossEmptyLinesAndComments``
-
-  For example, to align across empty lines and not across comments, either
-  of these work.
-
-  .. code-block:: c++
-
-    AlignConsecutiveMacros: AcrossEmptyLines
-
-    AlignConsecutiveMacros:
-      Enabled: true
-      AcrossEmptyLines: true
-      AcrossComments: false
-
-  * ``bool Enabled`` Whether aligning is enabled.
-
-    .. code-block:: c++
-
-      #define SHORT_NAME       42
-      #define LONGER_NAME      0x007f
-      #define EVEN_LONGER_NAME (2)
-      #define foo(x)           (x * x)
-      #define bar(y, z)        (y + z)
-
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int aaaa : 1;
-      int b    : 12;
-      int ccc  : 8;
-
-      int         aaaa = 12;
-      float       b = 23;
-      std::string ccc;
-
-  * ``bool AcrossEmptyLines`` Whether to align across empty lines.
-
-    .. code-block:: c++
-
-      true:
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int d            = 3;
-
-      false:
-      int a            = 1;
-      int somelongname = 2;
-      double c         = 3;
-
-      int d = 3;
-
-  * ``bool AcrossComments`` Whether to align across comments.
-
-    .. code-block:: c++
-
-      true:
-      int d    = 3;
-      /* A comment. */
-      double e = 4;
-
-      false:
-      int d = 3;
-      /* A comment. */
-      double e = 4;
-
-  * ``bool AlignCompound`` Only for ``AlignConsecutiveAssignments``.  Whether compound assignments
-    like ``+=`` are aligned along with ``=``.
-
-    .. code-block:: c++
-
-      true:
-      a   &= 2;
-      bbb  = 2;
-
-      false:
-      a &= 2;
-      bbb = 2;
-
-  * ``bool AlignFunctionDeclarations`` Only for ``AlignConsecutiveDeclarations``. Whether function declarations
-    are aligned.
-
-    .. code-block:: c++
-
-      true:
-      unsigned int f1(void);
-      void         f2(void);
-      size_t       f3(void);
-
-      false:
-      unsigned int f1(void);
-      void f2(void);
-      size_t f3(void);
-
-  * ``bool AlignFunctionPointers`` Only for ``AlignConsecutiveDeclarations``. Whether function pointers are
-    aligned.
-
-    .. code-block:: c++
-
-      true:
-      unsigned i;
-      int     &r;
-      int     *p;
-      int      (*f)();
-
-      false:
-      unsigned i;
-      int     &r;
-      int     *p;
-      int (*f)();
-
-  * ``bool PadOperators`` Only for ``AlignConsecutiveAssignments``.  Whether short assignment
-    operators are left-padded to the same length as long ones in order to
-    put all assignment operators to the right of the left hand side.
-
-    .. code-block:: c++
-
-      true:
-      a   >>= 2;
-      bbb   = 2;
-
-      a     = 2;
-      bbb >>= 2;
-
-      false:
-      a >>= 2;
-      bbb = 2;
-
-      a     = 2;
-      bbb >>= 2;
-
-
-.. _AlignConsecutiveShortCaseStatements:
-
-**AlignConsecutiveShortCaseStatements** (``ShortCaseStatementsAlignmentStyle``) :versionbadge:`clang-format 17` :ref:`¶ <AlignConsecutiveShortCaseStatements>`
-  Style of aligning consecutive short case labels.
-  Only applies if ``AllowShortCaseExpressionOnASingleLine`` or
-  ``AllowShortCaseLabelsOnASingleLine`` is ``true``.
-
-
-  .. code-block:: yaml
-
-    # Example of usage:
-    AlignConsecutiveShortCaseStatements:
-      Enabled: true
-      AcrossEmptyLines: true
-      AcrossComments: true
-      AlignCaseColons: false
-
-  Nested configuration flags:
-
-  Alignment options.
-
-  * ``bool Enabled`` Whether aligning is enabled.
-
-    .. code-block:: c++
-
-      true:
-      switch (level) {
-      case log::info:    return "info:";
-      case log::warning: return "warning:";
-      default:           return "";
-      }
-
-      false:
-      switch (level) {
-      case log::info: return "info:";
-      case log::warning: return "warning:";
-      default: return "";
-      }
-
-  * ``bool AcrossEmptyLines`` Whether to align across empty lines.
-
-    .. code-block:: c++
-
-      true:
-      switch (level) {
-      case log::info:    return "info:";
-      case log::warning: return "warning:";
-
-      default:           return "";
-      }
-
-      false:
-      switch (level) {
-      case log::info:    return "info:";
-      case log::warning: return "warning:";
-
-      default: return "";
-      }
-
-  * ``bool AcrossComments`` Whether to align across comments.
-
-    .. code-block:: c++
-
-      true:
-      switch (level) {
-      case log::info:    return "info:";
-      case log::w...
[truncated]

@boomanaiden154 boomanaiden154 requested a review from owenca October 31, 2024 05:54
@owenca
Copy link
Contributor

owenca commented Nov 1, 2024

@boomanaiden154 can we have clang-format-style-options rebuilt only when it's outdated, i.e. only if at least one of the following has been updated?

clang/include/clang/Format/Format.h
clang/include/clang/Tooling/Inclusions/IncludeStyle.h
clang/docs/ClangFormatStyleOptions.rst
clang/docs/tools/dump_format_style.py

See #111513 (comment).

@boomanaiden154
Copy link
Contributor Author

can we have clang-format-style-options rebuilt only when it's outdated, i.e. only if at least one of the following has been updated?

We can, but it requires more changes that I'm not sure are worth doing given that this step takes essentially zero time compared to regenerating the docs with Sphinx, which already does run every time.

@owenca
Copy link
Contributor

owenca commented Nov 7, 2024

can we have clang-format-style-options rebuilt only when it's outdated, i.e. only if at least one of the following has been updated?

We can, but it requires more changes that I'm not sure are worth doing given that this step takes essentially zero time compared to regenerating the docs with Sphinx, which already does run every time.

I'm not concerned about how much time it takes to run. I want to add it to clangFormat's dependencies if it doesn't run every time I do ninja FormatTests. Can something similar to https://github.com/llvm/llvm-project/pull/111513/files#r1832152957 be done here?

@boomanaiden154
Copy link
Contributor Author

I'm not concerned about how much time it takes to run. I want to add it to clangFormat's dependencies if it doesn't run every time I do ninja FormatTests.

I've updated the patch so that we only rerun the python script if the dependencies change. I've added a .template extension to the docs file so that it doesn't interfere with the copy in the build directory. It does not seem like there's an easy way to get CMake to copy the docs directory to the build directory while excluding that individual file. So renaming it prevents Sphinx from picking up the templatized version.

What's the point of adding it to the FormatTests target though? Just to ensure proper test coverage of the dump_format_style.py file? This seems like something that more should be covered by the documentation builds, which occur both precommit and postcommit with the precommit builds having reasonably nice UI.

Copy link
Contributor

@mydeveloperday mydeveloperday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so here now lie a problem, we'll not "see the ClangFormatStyleOptions.rst" during the review, so we'll not be able to sanity check the code in Format.h has been correctly generated. I feel this is a backward step and will likely lead to invalid documentation.

I'd personally like a solution that doesn't remove the ClangFormatStyleOptions.rst from the review

@boomanaiden154
Copy link
Contributor Author

I'd personally like a solution that doesn't remove the ClangFormatStyleOptions.rst from the review

Could we just test that the output looks as expected? I think it would be pretty easy to write a lit test that asserts all of the output is as expected. That would then show up in review.

It would be a bit of a hack, but it would solve the original problem, making sure that the docs actually get updated with the script automatically, and that things show up in review.

@mydeveloperday
Copy link
Contributor

I'd personally like a solution that doesn't remove the ClangFormatStyleOptions.rst from the review

Could we just test that the output looks as expected? I think it would be pretty easy to write a lit test that asserts all of the output is as expected. That would then show up in review.

It would be a bit of a hack, but it would solve the original problem, making sure that the docs actually get updated with the script automatically, and that things show up in review.

As a reviewer that means we have to download the review, rebuild the rst then check it. I don't quite understand the problem we are trying to solve here? Given that the python script unpacks the Format.h and makes the rst I would prefer to see the finished rst as part of the review. Sorry this doesn't fit your agenda but your not the one doing the reviews!

@owenca
Copy link
Contributor

owenca commented Nov 30, 2024

As a reviewer that means we have to download the review, rebuild the rst then check it. I don't quite understand the problem we are trying to solve here? Given that the python script unpacks the Format.h and makes the rst I would prefer to see the finished rst as part of the review.

Whenever I review a pull request that has edited Format.h, I always fetch the PR and run the python script to ensure that the generated rst file matches the one in git. I don't think there's a way around that. So I'm ok with reducing the current rst file to a template and removing thousands of lines from the repo.

The alternative would be #111513, which leaves the rst file in tact.

@boomanaiden154
Copy link
Contributor Author

Whenever I review a pull request that has edited Format.h, I always fetch the PR and run the python script to ensure that the generated rst file matches the one in git. I don't think there's a way around that.

Thinking about this a bit more, I think we can probably satisfy all the constraints if we write a lit test that ensures the in-tree ClangFormatStyleOptions.rst matches what the script will produce (#118154)? That solves the original problem that #111513 set out to solve (if I remember correctly) and will show up as a failure in CI if not done.

That doesn't get rid of the generated code being in the repo, but it seems like that is an essential part of some review workflows currently.

I'm also planning on making the documentation build action upload the built docs as artifacts, which would also let reviewers just download the built HTML files and inspect those if that's easier/useful.

@owenca
Copy link
Contributor

owenca commented Dec 1, 2024

What's the point of adding it to the FormatTests target though? Just to ensure proper test coverage of the dump_format_style.py file?

That and to also ensure that the edited Format.h doesn't break the python script.

Thinking about this a bit more, I think we can probably satisfy all the constraints if we write a lit test that ensures the in-tree ClangFormatStyleOptions.rst matches what the script will produce (#118154)? That solves the original problem that #111513 set out to solve (if I remember correctly) and will show up as a failure in CI if not done.

If we are to leave the generated part of the rst file in the repo, #111513 would satisfy my requirements nicely. Something like #118154 may be a useful addition.

I'm also planning on making the documentation build action upload the built docs as artifacts, which would also let reviewers just download the built HTML files and inspect those if that's easier/useful.

That would be nice.

@boomanaiden154
Copy link
Contributor Author

That and to also ensure that the edited Format.h doesn't break the python script.

Makes sense. #118154 would cover that too.

If we are to leave the generated part of the rst file in the repo, #111513 would satisfy my requirements nicely. Something like #118154 may be a useful addition.

Can you explicate on what your requirements are here? I don't see why just wrapping the python script in CMake does anything tangible here. Having a build target that modifies the source directory is also weird and something that we should avoid if possible, in my opinion.

That would be nice.

#118159

@owenca
Copy link
Contributor

owenca commented Dec 1, 2024

Can you explicate on what your requirements are here? I don't see why just wrapping the python script in CMake does anything tangible here. Having a build target that modifies the source directory is also weird and something that we should avoid if possible, in my opinion.

From #111513 (comment):

Anyway, my current workflow is:

  1. Edit files in clang/lib/Format and clang/unittests/Format.
  2. Edit clang/include/clang/Format/Format.h if needed.
  3. Run ninja FormatTests.
  4. Run cd clang/docs/tools && dump_format_style.py if Step 2 above wasn't skipped.
  5. Run the unit tests.
  6. Run git commit -a.

It would be nice if I didn't have to run step 4 manually.

With #111513, I no longer need to remind myself to run the python script when testing a patch. Please note that we rarely need to do ninja check-clang-format to run the lit tests, which takes an extra hour or two.

@boomanaiden154
Copy link
Contributor Author

Closing in favor of #111513 / #118154.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category clang-format
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants