Bug 17124 – Use a script to make style more consistent throughout WebCore/JavaScriptCore

• Status: RESOLVED
• Resolution: WONTFIX
• Component: New Bugs

[Eric Seidel (no email)]

Use a script to make style more consistent throughout WebCore/JavaScriptCore

Script coming up. :)

[Eric Seidel (no email)]

Created attachment 18832 [details]
script to do all the fixups

The resulting patch is 1.4mb (smaller than I thought it would be).  So I may need to break each of these script runs into separate stages, or find some other nice way to get a review.

[Eric Seidel (no email)]

Created attachment 18833 [details]
files affected by the script

The first question to have answered is if all of these files should be changing (qt, gtk, for instance).  Here is a full list of files that this script changes.

[Eric Seidel (no email)]

Created attachment 18834

[Eric Seidel (no email)]

One change I could make to the script is to have it completely ignore things after //, since there are a few cases in this diff where comments messed up.  I'm not sure it's worth it.

[Alp Toker]

Comment on attachment 18834


The NULLs in the GTK+ part of WebCore probably shouldn't be changed. NULL parameters are common in GObject and using 0 makes calls difficult to read. This is similar to the coding style exceptions made in the Mac Obj-C code.

I haven't worked with Win32 for some years but in my experience Win32 hackers will prefer to use NULL when calling into Win32, so you might want to avoid the s/NULL/0 cleanup for those too.


Some things the script missed:

Weird whitespace still there after second ):

-    switch ( readResult)    {
+    switch (readResult)    {

Position of first & not fixed:

-ImageDecoderQt::ReadContext::ReadContext(const IncomingData & data, LoadMode loadMode, ImageList &target)
+ImageDecoderQt::ReadContext::ReadContext(const IncomingData & data, LoadMode loadMode, ImageList& target)


Whitespace not fixed entirely:

-  gettimeofday( &aTimeval, &aTimezone );
-  return (double)aTimeval.tv_sec + (double)(aTimeval.tv_usec / 1000000.0 );
+  gettimeofday( &aTimeval, &aTimezone);
+  return (double)aTimeval.tv_sec + (double)(aTimeval.tv_usec / 1000000.0);

I didn't keep looking, can do a more full review if you need.

Maybe a combination of astyle and your tool will help catch more cases? It certainly makes sense to do as much cleanup as we can in one go.

I remember astyle caught some of the cases yours doesn't, and yours covers some cases that astyle is bad with. There's also WebKitTools/Scripts/wkstyle which may have some ideas.

Thanks for looking into this Eric!

[Darin Adler]

(In reply to comment #5)
> I haven't worked with Win32 for some years but in my experience Win32 hackers
> will prefer to use NULL when calling into Win32, so you might want to avoid the
> s/NULL/0 cleanup for those too.

Nah, I think changing NULL to 0 is fine for Win32.

[Eric Seidel (no email)]

As much as I would love it to be, this was not meant to be a complete fix for all style.  I appreciate you pointing out the cases I missed, I'll see if I can enhance the script.

I think in order to make this easier to work with, I will enhance the script to take a directory/filename arg, and then land the script and do one subdirectory at a time.  Those will be much smaller and easier to review.  We can also turn on/off options based on directories and over time improve the script to catch more bad style cases.

[Alp Toker]

(In reply to comment #7)
> As much as I would love it to be, this was not meant to be a complete fix for
> all style.  I appreciate you pointing out the cases I missed, I'll see if I can
> enhance the script.

Understood, running the whole codebase through astyle would probably be a much larger diff.

I looked over the changes a bit and noticed one more thing. Bundled code like WebCore/platform/image-decoders/zlib shouldn't be modified otherwise it'll diverge from the upstream project and become difficult to track our local changes with diff.

I think this might also apply to things like npapi.h. It's quite useful to be able to diff our npapi.h against Mozilla's to track what changes we have.


Oops:

-         range if this entry defines a range, OR the *signed* offset to the
+         range if this entry defines a range, OR the* signed* offset to the

-  // REQUIRES: lock_ is *not* held.
+  // REQUIRES: lock_ is* not* held.

-// avoid having to turn all the quote marks into " as we would have to.
+// avoid having to turn all the quote marks into& quot; as we would have to.



I think changes to comments like this make them ambiguous:

-    // Returns the DOM ownerDocument attribute. This method never returns NULL, except in the case 
+    // Returns the DOM ownerDocument attribute. This method never returns 0, except in the case
     // of (1) a Document node or (2) a DocumentType node that is not used with any Document yet. 
     virtual Document* ownerDocument() const;

^ The original comment is very clear at explaining that the function will return a NULL pointer. The updated text is ambiguous -- it now sounds to me like the function will return a an attribute, but the attribute will have value "0".

So there is a loss of delicate semantics here and in a bunch of other places.

[Eric Seidel (no email)]

Comment on attachment 18832 [details]
script to do all the fixups

As I commented above, I'm going to make this take directory names as args and do these fixups one group of files at a time.

[David Kilzer (:ddkilzer)]

See also Bug 9671.

[Martin Robinson]

Is this something that's superceded by the style bot?

[Alexey Proskuryakov]

I think that for a long time now, the approach has been to not fix style unless you touch the code for other reasons. WONTFIX?

[Eric Seidel (no email)]

Yes, this is effectively superseded by the style bot.