[libcamera-devel] [PATCH v2] clang-format: Regroup sort orders
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Aug 9 19:09:10 CEST 2021
Hi Kieran,
On Mon, Aug 09, 2021 at 06:05:36PM +0100, Kieran Bingham wrote:
> On 09/08/2021 17:36, Laurent Pinchart wrote:
> > 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 ?
>
> Intentionally, yes as stated in the three lines above your query " (+ a
> couple of extra tests)"
Oops indeed.
> >> 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.
>
> We would have to add that set explicitly. That doesn't seem to scale, as
> there would likely always be some extra files we'd have to add, and I
> don't think we should be maintaining a list of 'all files' in a regex.
As we have few external dependencies I thought this list may change
infrequently enough that it could be maintainable (especially
considering that most external dependencies will have a directory name
as a prefix), but I also understand the opposite argument.
> As you'll see, I've added an exception rule for the QT headers, as it
> can match generically to prevent QT headers ending up in the C++ subset.
>
> > I've also spotted
> >
> > #include "linux/bcm2835-isp.h"
> >
> > that should be replaced with
> >
> > #include <linux/bcm2835-isp.h>
>
> Sure - but that's a separate patch ;-) now sent.
Thanks.
> >> .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 ?
>
> At the moment just matching them. Though I thought we did so already
> somewhere... but I can't find any examples of that, but there are plenty
> of examples of them mixed, so I will re-join groups 2 and 3.
Ack.
> >> +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 ?
>
> It aims to better match what current expected practice is ... at least
> how I see it ...
>
> Specifically - the base interfaces are all described like that in the
> implementation, because I used a variant of this .clang-format to help
> automate that split work.
>
> There are lots of rules in the headers already (like
> libcamera/base/private.h should be in it's own group, before the other
> base headers) which this patch codifies.
>
> There are many times where it's not easily obvious where an include
> should go. This tries to specify that for the readers, and ... make it
> so they don't have to look it up anyway, as the tooling will organise it
> anyway.
Fine with me. Could you mention in the commit message that the rules are
created to match the existing practices, and that the documentation is
updated accordingly ? With that,
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> 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
More information about the libcamera-devel
mailing list