Bug 141872 - check-webkit-style: Add exception for FrameworkSoftLink.h header order
Summary: check-webkit-style: Add exception for FrameworkSoftLink.h header order
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on:
Blocks: 141625
  Show dependency treegraph
 
Reported: 2015-02-21 17:22 PST by David Kilzer (:ddkilzer)
Modified: 2015-03-03 18:48 PST (History)
9 users (show)

See Also:


Attachments
Patch v1 (3.20 KB, patch)
2015-02-21 17:25 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (13.23 KB, patch)
2015-03-02 19:39 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2015-02-21 17:22:16 PST
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.
Comment 1 David Kilzer (:ddkilzer) 2015-02-21 17:25:52 PST
Created attachment 247068 [details]
Patch v1
Comment 2 Alex Christensen 2015-02-23 12:06:03 PST
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?
Comment 3 David Kilzer (:ddkilzer) 2015-02-23 18:57:07 PST
(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.
Comment 4 David Kilzer (:ddkilzer) 2015-03-02 19:39:32 PST
Created attachment 247738 [details]
Patch v2
Comment 5 David Kilzer (:ddkilzer) 2015-03-02 19:44:09 PST
(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]).
Comment 6 Alex Christensen 2015-03-03 10:17:16 PST
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?
Comment 7 David Kilzer (:ddkilzer) 2015-03-03 16:18:56 PST
(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.
Comment 8 WebKit Commit Bot 2015-03-03 18:48:17 PST
Comment on attachment 247738 [details]
Patch v2

Clearing flags on attachment: 247738

Committed r180981: <http://trac.webkit.org/changeset/180981>
Comment 9 WebKit Commit Bot 2015-03-03 18:48:22 PST
All reviewed patches have been landed.  Closing bug.