[libcamera-devel] [PATCH 1/2] utils: checkstyle: Add formatter to sort #include statements

[Kieran Bingham]

Hi Laurent,

On 23/03/2020 11:41, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Mon, Mar 23, 2020 at 11:35:39AM +0000, Kieran Bingham wrote:
>> On 22/03/2020 12:02, Laurent Pinchart wrote:
>>
>> Ha, I love this :-)
>>
>> One less thing to worry about in reviews!
>>
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>> ---
>>>  utils/checkstyle.py | 33 +++++++++++++++++++++++++++++++++
>>>  1 file changed, 33 insertions(+)
>>>
>>> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
>>> index 0827a1e6ba0f..5fd63f047781 100755
>>> --- a/utils/checkstyle.py
>>> +++ b/utils/checkstyle.py
>>> @@ -481,6 +481,39 @@ class DoxygenFormatter(Formatter):
>>>          return '\n'.join(lines)
>>>  
>>>  
>>> +class IncludeOrderFormatter(Formatter):
>>> +    patterns = ('*.cpp', '*.h')
>>> +
>>> +    include_regex = re.compile('^#include ["<]([^">]*)[">]')
>>> +
>>> +    @classmethod
>>> +    def format(cls, filename, data):
>>> +        lines = []
>>> +        includes = []
>>> +
>>> +        for line in data.split('\n'):
>>> +            match = IncludeOrderFormatter.include_regex.match(line)
>>> +            if match:
>>> +                includes.append((line, match.group(1)))
>>> +                continue
>>> +
>>
>> Maybe it's just me, but this seems somehow quite hard to interpret and I
>> found it difficult to work out /how/ the code is parsing the blocks.
>>
>> I'm not yet sure what, but a comment describing what happens somewhere
>> would be useful for the future me...
>>
>> (It didn't help that I missed the continue on my first read, so I didn't
>> realise the code below was only executed for non #include lines).
>>
>> Maybe a top level comment saying:
>>  # Parse blocks of #include statements, and output them as a sorted list
>>  # when we reach a non #include statement.
>>
>>
>> Perhaps that's verging towards commenting what the code does rather than
>> why, but now that I 'know' that information, it's easier to parse the
>> paths of the code to determine the 'how'...
> 
> diff --git a/utils/checkstyle.py b/utils/checkstyle.py
> index 5fd63f047781..b594a19aed5b 100755
> --- a/utils/checkstyle.py
> +++ b/utils/checkstyle.py
> @@ -491,12 +491,18 @@ class IncludeOrderFormatter(Formatter):
>          lines = []
>          includes = []
> 
> +        # Parse blocks of #include statements, and output them as a sorted list
> +        # when we reach a non #include statement.
>          for line in data.split('\n'):
>              match = IncludeOrderFormatter.include_regex.match(line)
>              if match:
> +                # If the current line is an #include statement, add it to the
> +                # includes group and continue to the next line.
>                  includes.append((line, match.group(1)))
>                  continue
> 
> +            # The current line is not an #include statement, output the sorted
> +            # stashed includes first, and then the current line.
>              if len(includes):
>                  includes.sort(key=lambda i: i[1])
>                  for include in includes:
> @@ -505,6 +511,8 @@ class IncludeOrderFormatter(Formatter):
> 
>              lines.append(line)
> 
> +        # In the unlikely case the file ends with an #include statement, make
> +        # sure we output the stashed includes.
>          if len(includes):
>              includes.sort(key=lambda i: i[1])
>              for include in includes:
> 
> Does this look good to you ? If so, your Rb tag will be appreciated (for
> the result of squashing the original patch with this change) :-)

Haha - more than I was expecting -  but that's helpful in this case I
think :-)

Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

> 
>>> +            if len(includes):
>>> +                includes.sort(key=lambda i: i[1])
>>> +                for include in includes:
>>> +                    lines.append(include[0])
>>> +                includes = []
>>> +
>>> +            lines.append(line)
>>> +
>>> +        if len(includes):
>>> +            includes.sort(key=lambda i: i[1])
>>> +            for include in includes:
>>> +                lines.append(include[0])
>>> +            includes = []
>>> +
>>> +        return '\n'.join(lines)
>>> +
>>> +
>>>  class StripTrailingSpaceFormatter(Formatter):
>>>      patterns = ('*.c', '*.cpp', '*.h', '*.py', 'meson.build')
>>>  
>>>
> 

-- 
Regards
--
Kieran