Bug 27524 – Add check for correct wtf includes to cpplint

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jakob Petsovits]

Adam Treat told me that wtf includes in WebCore should be included using <wtf/file.h> instead of "wtf/file.h". Here's a check for that, hopefully it's how it should approximately be done.

[Jakob Petsovits]

Created attachment 33225 [details]
Add check for correct wtf includes in WebCore to cpplint

The patch implementing this check.

[David Levin]

Comment on attachment 33225 [details]
Add check for correct wtf includes in WebCore to cpplint

> diff --git a/WebKitTools/Scripts/modules/cpplint.py b/WebKitTools/Scripts/modules/cpplint.py
> +        # Check specific include file rules.
> +        if filename != "-":

When is the filename "-" ?

> +            filepath = os.path.abspath(filename)
> +            filepath_parts = filepath.split(os.sep)
> +            if 'WebCore' in filepath_parts:

Why only WebCore? I think this is true in all directories. (And if it should only be WebCore, please add a test for this.)



>

[Adam Treat]

After talking with Mark Rowe and Dave Levin on IRC it would seem we don't need
to detect whether the file is in WebCore or not.  All references to a wtf
header of the form wtf/foo.h should always be <> style, not "" style.  No
matter what file is doing the including.

[Jakob Petsovits]

Created attachment 33261 [details]
Add check for correct wtf includes to cpplint

re #3: Cool, that simplifies stuff a lot. Also looks less out of place this way. Please re-review... thanks!

[Adam Treat]

Comment on attachment 33261 [details]
Add check for correct wtf includes to cpplint

                 'wtf includes inside WebCore should be <wtf/file.h> instead of "wtf/file.h".')

This should no longer mention 'WebCore'.  Just say, "wtf includes should be..."

And make sure to change the tests to reflect :)

I'd tell you to make the change on landing, but you can't land yet so please post another patch with that change and then I'll land.

[Jakob Petsovits]

Created attachment 33263 [details]
Add check for correct wtf includes to cpplint

Ouch. I adapted the ChangeLog, the commit message and the test function, but that occurrence I forgot... fixed in this version, no other changes.

[Adam Treat]

Landed with r46225.

[Darin Adler]

An exception would be includes of wtf from files inside wtf itself. These should use the "HashMap.h" style in both headers and implementation files.

[Adam Treat]

(In reply to comment #8)
> An exception would be includes of wtf from files inside wtf itself. These
> should use the "HashMap.h" style in both headers and implementation files.

Talked about this extensively with Mark Rowe and David Levin on IRC.  Currently, JavaScriptCore/wtf/Foo.h uses <wtf/Bar.h> all over the place.  In fact, one of the things that informed us was a commit by you changing one of the files from "" to <>.

[Darin Adler]

(In reply to comment #9)
> Talked about this extensively with Mark Rowe and David Levin on IRC. 
> Currently, JavaScriptCore/wtf/Foo.h uses <wtf/Bar.h> all over the place.  In
> fact, one of the things that informed us was a commit by you changing one of
> the files from "" to <>.

OK, my bad, but that won’t work on the Mac if we ever decide to make some of the wtf headers be public headers in the JavaScriptCore framework. It would force us to name the framework "wtf".

Mark Rowe is definitely an expert in this area, though.