Last Comment Bug 688012 - Fix the most egregious xpconnect formatting abnormalities
: Fix the most egregious xpconnect formatting abnormalities
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Bobby Holley (:bholley) (busy with Stylo)
:
Mentors:
Depends on: 691411
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-20 14:38 PDT by Bobby Holley (:bholley) (busy with Stylo)
Modified: 2011-10-14 16:54 PDT (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
xpcfix v1 (8.56 KB, text/plain)
2011-10-13 09:37 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details
patches for mrbkap to review (63 bytes, text/plain)
2011-10-13 09:43 PDT, Bobby Holley (:bholley) (busy with Stylo)
mrbkap: review+
Details
xpcfix v2 (10.07 KB, text/plain)
2011-10-13 18:46 PDT, Bobby Holley (:bholley) (busy with Stylo)
no flags Details

Description Bobby Holley (:bholley) (busy with Stylo) 2011-09-20 14:38:43 PDT
mrbkap wants js-engine style, which I think should be pretty doable with astyle. This would be good to do a bit after the branch.
Comment 1 Bobby Holley (:bholley) (busy with Stylo) 2011-09-30 16:15:05 PDT
I spent part of today investigating this. The long and short of it is there doesn't seem to be a good tool that will make just the changes we want and none more. uncrustify comes very close, but the core re-indenting algorithm is non-optional. In the end, there were too many cases where it didn't get one of our idioms quite right, in ways that I was unable to rectify using the (hundreds of) built-in configuration parameters.

So I'm going to apply the 80/20 principle here, and just write quick scripts to fix some of the most egregious issues. Here are my current plans:

1 - Nuke a few dead directories (sample/ and tests/js/).
2 - Remove trailing whitespace.
3 - Replace if( with if (.
4 - Move braces on if/while/for loops from their own line to the line of the conditional.

1 is easy. 2 is easy. 3 and 4 require a little care to keep other things lined up right, but overall shouldn't be too hard.

The nice thing is that these changes are all whitespace-only, so we can very easily verify that no behavior is changed.

I'm tempted to capitalize the filenames, but maybe that can wait until another day.

I'm aware that some of the people CCed might have pending patches in this code. If you do, perhaps we can coordinate to avoid too much PITA?

I'm also aware that this might seem like unnecessary churn. But I contend that the "if(" thing is a real problem. If we want to maintain some semblance of stylistic consistency, we have to bounce almost every patch on initial review, since nobody else in Mozilla writes code like that. This can amount to a lot of wasted developer time, and seems pretty suboptimal. :\

How does this all sound? Does anyone have anything else along these lines that they'd like done while I'm at it?
Comment 2 Luke Wagner [:luke] 2011-09-30 16:27:22 PDT
Did I see something about js/xpconnect on irc? ;-)
Comment 3 Bobby Holley (:bholley) (busy with Stylo) 2011-09-30 16:29:10 PDT
(In reply to Luke Wagner [:luke] from comment #2)
> Did I see something about js/xpconnect on irc? ;-)

Sure, I can do that. :-)
Comment 4 Bobby Holley (:bholley) (busy with Stylo) 2011-10-03 10:53:03 PDT
(In reply to Bobby Holley (:bholley) from comment #3)
> (In reply to Luke Wagner [:luke] from comment #2)
> > Did I see something about js/xpconnect on irc? ;-)
> 
> Sure, I can do that. :-)

I decided that it makes more sense to spin this off into its own bug, so I filed bug 691411. I'm still going to do this on top of that one, but they're logically separate tasks.
Comment 5 Bobby Holley (:bholley) (busy with Stylo) 2011-10-13 09:37:11 PDT
Created attachment 566864 [details]
xpcfix v1

So I've built a tool to make these adjustments. It was largely an interactive process of running it, seeing where it got confused, and teaching it about more special cases. It ended up being rather complex. ;-)

Overall it works pretty well - occasionally it makes a change that isn't quite ideal, but I think its output is, without question, preferable to its input.

I've attached the tool for the curious.
Comment 6 Bobby Holley (:bholley) (busy with Stylo) 2011-10-13 09:43:28 PDT
Created attachment 566865 [details]
patches for mrbkap to review

I've applied xpcfix to do the restyle. This is a very large and breakable set of patches (since they touch everything), so I'm really hoping we can expedite the review/landing process. ;-)

The patches are here:
https://github.com/bholley/mozilla-central/commits/xpc_reformat

Each one contains the xpcfix command used to generate it, along with some explanation as to what's going on.

Flagging mrbkap for review. Blake, let me know if there's anything I can do to simplify the review process (I'm not sure how you want to go about it). If you occasionally run across a special case of a restyling you don't like, please keep a list. Assuming it's small enough, it's probably easier to add a fixup patch on top of everything than to rework the tooling.
Comment 7 Blake Kaplan (:mrbkap) 2011-10-13 16:54:20 PDT
Comment on attachment 566865 [details]
patches for mrbkap to review

This looks pretty good. I talked to bholley on IRC about three things I noticed:

When a static_cast<>() overflows, the ( should hang on the next line and line up with the < on the previous line.

We're going to pull the elses up to cuddle with the } for the preceding if statement.

There are a couple of cases where the script accidentally pulls up scoping opening braces when they should be left on their own line.

bholley is going to fix these problems as well as fixing the opening brace for else.
Comment 8 Bobby Holley (:bholley) (busy with Stylo) 2011-10-13 18:46:41 PDT
Created attachment 566997 [details]
xpcfix v2

Addressed mrbkap's review comments. The github branch has been updated: https://github.com/bholley/mozilla-central/commits/xpc_reformat

A few changes had to be made to xpcfix as well (attached).

I pushed this try: https://tbpl.mozilla.org/?tree=Try&rev=69abf8a9cd05

If it goes green, I'll push it tomorrow morning.
Comment 9 Bobby Holley (:bholley) (busy with Stylo) 2011-10-13 22:57:02 PDT
Doh, mccr8's recent push added a new test file that wasn't in my move patch.

Take two: https://tbpl.mozilla.org/?tree=Try&rev=8b06f5953787
Comment 10 Bobby Holley (:bholley) (busy with Stylo) 2011-10-14 16:54:51 PDT
This landed to mozilla-central, and after some drama and excitement, stuck:

http://hg.mozilla.org/mozilla-central/rev/921f44191dcb
(and ancestors)

Resolving fixed. \o/

Note You need to log in before you can comment on or make changes to this bug.