Use a script to make style more consistent throughout WebCore/JavaScriptCore Script coming up. :)
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.
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.
Created attachment 18834
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.
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!
(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.
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.
(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.
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.
See also Bug 9671.
Is this something that's superceded by the style bot?
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?
Yes, this is effectively superseded by the style bot.