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

[Laurent Pinchart]

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) :-)

> > +            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,

Laurent Pinchart