Bug 27524 - Add check for correct wtf includes to cpplint
Summary: Add check for correct wtf includes to cpplint
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-07-21 16:00 PDT by Jakob Petsovits
Modified: 2009-07-22 12:28 PDT (History)
2 users (show)

See Also:


Attachments
Add check for correct wtf includes in WebCore to cpplint (4.29 KB, patch)
2009-07-21 16:14 PDT, Jakob Petsovits
levin: review-
Details | Formatted Diff | Diff
Add check for correct wtf includes to cpplint (4.00 KB, patch)
2009-07-22 07:33 PDT, Jakob Petsovits
manyoso: review-
Details | Formatted Diff | Diff
Add check for correct wtf includes to cpplint (3.98 KB, patch)
2009-07-22 07:54 PDT, Jakob Petsovits
manyoso: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jakob Petsovits 2009-07-21 16:00:18 PDT
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.
Comment 1 Jakob Petsovits 2009-07-21 16:14:03 PDT
Created attachment 33225 [details]
Add check for correct wtf includes in WebCore to cpplint

The patch implementing this check.
Comment 2 David Levin 2009-07-21 16:47:31 PDT
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.)



>
Comment 3 Adam Treat 2009-07-21 17:37:11 PDT
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.
Comment 4 Jakob Petsovits 2009-07-22 07:33:58 PDT
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!
Comment 5 Adam Treat 2009-07-22 07:49:49 PDT
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.
Comment 6 Jakob Petsovits 2009-07-22 07:54:28 PDT
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.
Comment 7 Adam Treat 2009-07-22 08:20:58 PDT
Landed with r46225.
Comment 8 Darin Adler 2009-07-22 11:33:09 PDT
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.
Comment 9 Adam Treat 2009-07-22 11:38:50 PDT
(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 <>.
Comment 10 Darin Adler 2009-07-22 12:28:27 PDT
(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.