[libcamera-devel] [PATCH v2] clang-format: Regroup sort orders
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Aug 9 19:13:18 CEST 2021
Hi Laurent,
On 09/08/2021 18:09, Laurent Pinchart wrote:
> 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.
Indeed, dependencies with a directory name are better, as they already
get matched to group 10 (9, now that I've merged C and C++).
>> 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.
Done, but somehow in the big merged list - it looks ugly ;-)
I liked seeing them split.
>
>>>> +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,
Sure.
Thanks.
>
> 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
>
More information about the libcamera-devel
mailing list