[libcamera-devel] [PATCH v2] clang-format: Regroup sort orders

[Laurent Pinchart]

Hi Kieran,

Thank you for the patch.

On Mon, Aug 09, 2021 at 05:14:25PM +0100, Kieran Bingham wrote:
> Utilise the clang-format header sort to provide a regex based pattern
> match for our header inclusion coding style.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> ---
> A sample of all headers from the project (+ a couple of extra tests)
> parsed through this can be seen at 
>    https://paste.ubuntu.com/p/Bt87C6M77y/

There are a few headers there such as a.h and b.h that don't seem to
exist in libcamera. Did you run the commands below on a test branch by
any chance ?

> This was generated by:
>  git grep -h "^#include" | sort | uniq  > all-headers.cpp
>  git add all-headers.cpp
>  git commit all-headers.cpp -m "Header Test"
>  ./utils/checkstyle.py | patch -p0
> 
> While some false positives are visible in this list
>  (for instance <camera_metadata_hidden.h> in c/system headers section)
> this provides a great deal of the checks automatically, and the
> remaining cases may have to be spotted during review.

There's also

#include <jpeglib.h>
#include <libudev.h>
#include <tiffio.h>
#include <xf86drm.h>
#include <xf86drmMode.h>
#include <yaml.h>

in the system headers. I wonder if we should add a rule for those.

I've also spotted

#include "linux/bcm2835-isp.h"

that should be replaced with

#include <linux/bcm2835-isp.h>

>  .clang-format                  | 49 +++++++++++++++++++++++++++++++---
>  Documentation/coding-style.rst | 20 +++++++++++---
>  2 files changed, 62 insertions(+), 7 deletions(-)
> 
> diff --git a/.clang-format b/.clang-format
> index 3a8a896e373d..56cfbfa1bec4 100644
> --- a/.clang-format
> +++ b/.clang-format
> @@ -66,10 +66,53 @@ ExperimentalAutoDetectBinPacking: false
>  FixNamespaceComments: true
>  ForEachMacros:
>    - 'udev_list_entry_foreach'
> -IncludeBlocks: Preserve
> +SortIncludes: true
> +IncludeBlocks: Regroup
>  IncludeCategories:
> -  - Regex: '.*'
> -    Priority: 1
> +  # Headers matching the name of the component are matched automatically.
> +  # Priority 1
> +  # Headers in <> with an extension. (+system libraries)
> +  - Regex:           '<([A-Za-z0-9\-_])+\.h>'
> +    Priority:        2 
> +  - Regex:           '<sys/.*>'
> +    Priority:        2
> +  # Qt includes (match before C++ standard library)
> +  - Regex:           '<Q([A-Za-z0-9\-_])+>'
> +    Priority:        10
> +    CaseSensitive:   true
> +  # C++ standard library includes (no extension)
> +  - Regex:           '<([A-Za-z0-9\-_/])+>'
> +    Priority:        3
> +  # Linux headers, as a second group/subset of system headers
> +  - Regex:           '<linux/.*>'
> +    Priority:        4
> +  # Headers for libcamera Base support
> +  - Regex:           '<libcamera/base/private.h>'
> +    Priority:        5
> +  - Regex:           '<libcamera/base/.*\.h>'
> +    Priority:        6
> +  # Public API Headers for libcamera, which are not in a subdir (i.e. ipa/,internal/)
> +  - Regex:           '<libcamera/([A-Za-z0-9\-_])+.h>'
> +    Priority:        7
> +  # IPA Interfaces
> +  - Regex:           '<libcamera/ipa/.*\.h>'
> +    Priority:        8
> +  # libcamera Internal headers in ""  
> +  - Regex:           '"libcamera/internal/.*\.h"'
> +    Priority:        9
> +  # Other libraries headers with one group per library (.h or .hpp)
> +  - Regex:           '<.*/.*\.hp*>'
> +    Priority:        10
> +  # local modular includes "path/file.h" (.h or .hpp)
> +  - Regex:           '"(.*/)+.*\.hp*"'
> +    Priority:        11
> +  # Other local headers "file.h" with extension (.h or .hpp)
> +  - Regex:           '".*.hp*"'
> +    Priority:        12
> +  # Any unmatched line, separated from the last group
> +  - Regex:	     '"*"'
> +    Priority:        100
> +
>  IncludeIsMainRegex: '(_test)?$'
>  IndentCaseLabels: false
>  IndentPPDirectives: None
> diff --git a/Documentation/coding-style.rst b/Documentation/coding-style.rst
> index 30c1778ed8bf..e4f1bb26024e 100644
> --- a/Documentation/coding-style.rst
> +++ b/Documentation/coding-style.rst
> @@ -70,19 +70,31 @@ macro. For .cpp files, if the file implements an API declared in a header file,
>  that header file shall be included first in order to ensure it is
>  self-contained.
>  
> +While the following list is extensive, it documents the expected behaviour
> +defined by the clang-format configuraiton and tooling should assist with
> +ordering.
> +
>  The headers shall be grouped and ordered as follows:
>  
>  1. The header declaring the API being implemented (if any)
> -2. The C and C++ system and standard library headers
> -3. Other libraries' headers, with one group per library
> -4. Other project's headers
> +2. The C standard library and system headers
> +3. The C++ standard library headers

What's the rationale for splitting the C and C++ headers ?

> +4. Linux kernel headers
> +5. The libcamera base private header if required
> +6. The libcamera base library headers
> +7. The libcamera public API headers
> +8. The libcamera IPA interfaces
> +9. The internal libcamera headers
> +10. Other libraries' headers, with one group per library
> +11. Local headers grouped by subdirectory
> +12. Any local headers

There are substantial changes here, does this fix the documentation to
match what we actually do already, or does it change the current
practice ?

>  Groups of headers shall be separated by a single blank line. Headers within
>  each group shall be sorted alphabetically.
>  
>  System and library headers shall be included with angle brackets. Project
>  headers shall be included with angle brackets for the libcamera public API
> -headers, and with double quotes for other libcamera headers.
> +headers, and with double quotes for internal libcamera headers.
>  
>  
>  C++ Specific Rules

-- 
Regards,

Laurent Pinchart