Bug 141872 – check-webkit-style: Add exception for FrameworkSoftLink.h header order

• Component: Tools / Tests
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

Add an exception to check-webkit-style for the order of FrameworkSoftLink.h headers.  See Bug 141870, Comment #2 for an example.

These headers need to come last (after all other headers, but without #if/#endif protection) since they #define functions, constants, etc., and including them in alphabetical order with other headers may cause problems.

[David Kilzer (:ddkilzer)]

Created attachment 247068 [details]
Patch v1

[Alex Christensen]

Comment on attachment 247068 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=247068&action=review

This looks related to http://trac.webkit.org/changeset/180355

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3017
> +                    if not line.endswith('SoftLink.h"'):
> +                        error(line_number, 'build/include_order', 4, 'Bad include order. Mixing system and custom headers.')

This does not check to see if the SoftLink headers are last.  It just doesn't fail if they are last.  Should we add a check to make sure they are not mixed in with the other headers?

[David Kilzer (:ddkilzer)]

(In reply to comment #2)
> Comment on attachment 247068 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247068&action=review
> 
> This looks related to http://trac.webkit.org/changeset/180355
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:3017
> > +                    if not line.endswith('SoftLink.h"'):
> > +                        error(line_number, 'build/include_order', 4, 'Bad include order. Mixing system and custom headers.')
> 
> This does not check to see if the SoftLink headers are last.  It just
> doesn't fail if they are last.  Should we add a check to make sure they are
> not mixed in with the other headers?

Yeah, we should check that they're in the last group of headers.

[David Kilzer (:ddkilzer)]

Created attachment 247738 [details]
Patch v2

[David Kilzer (:ddkilzer)]

(In reply to comment #3)
> (In reply to comment #2)
> > Comment on attachment 247068 [details]
> > Patch v1
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=247068&action=review
> > 
> > This looks related to http://trac.webkit.org/changeset/180355
> > 
> > > Tools/Scripts/webkitpy/style/checkers/cpp.py:3017
> > > +                    if not line.endswith('SoftLink.h"'):
> > > +                        error(line_number, 'build/include_order', 4, 'Bad include order. Mixing system and custom headers.')
> > 
> > This does not check to see if the SoftLink headers are last.  It just
> > doesn't fail if they are last.  Should we add a check to make sure they are
> > not mixed in with the other headers?
> 
> Yeah, we should check that they're in the last group of headers.

To do this properly, I had to define a new header type and a new section type.  See Patch v2 (attachment 247738 [details]).

[Alex Christensen]

Comment on attachment 247738 [details]
Patch v2

View in context: https://bugs.webkit.org/attachment.cgi?id=247738&action=review

> Tools/Scripts/webkitpy/style/checkers/cpp.py:3013
> +        error(line_number, 'build/include_order', 4, error_message)

Does this 4 mean _SOFT_LINK_SECTION?

[David Kilzer (:ddkilzer)]

(In reply to comment #6)
> Comment on attachment 247738 [details]
> Patch v2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=247738&action=review
> 
> > Tools/Scripts/webkitpy/style/checkers/cpp.py:3013
> > +        error(line_number, 'build/include_order', 4, error_message)
> 
> Does this 4 mean _SOFT_LINK_SECTION?

No, it's the "error level" of the issue.  I'm just repeating the same error level for existing 'build/include_order' issues; presumably it's another way to filter style errors, although I've never used it that way.

[WebKit Commit Bot]

Comment on attachment 247738 [details]
Patch v2

Clearing flags on attachment: 247738

Committed r180981: <http://trac.webkit.org/changeset/180981>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.