Closed Bug 688012 Opened 10 years ago Closed 10 years ago

Fix the most egregious xpconnect formatting abnormalities


(Core :: XPConnect, defect)

Not set





(Reporter: bholley, Assigned: bholley)




(2 files, 1 obsolete file)

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.
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?
Summary: Run xpconnect through a source formatter → Fix the most egregious xpconnect formatting abnormalities
Did I see something about js/xpconnect on irc? ;-)
(In reply to Luke Wagner [:luke] from comment #2)
> Did I see something about js/xpconnect on irc? ;-)

Sure, I can do that. :-)
(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.
Depends on: 691411
Attached file xpcfix v1 (obsolete) —
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.
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:

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.
Attachment #566865 - Flags: review?(mrbkap)
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.
Attachment #566865 - Flags: review?(mrbkap) → review+
Attached file xpcfix v2
Addressed mrbkap's review comments. The github branch has been updated:

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

I pushed this try:

If it goes green, I'll push it tomorrow morning.
Attachment #566864 - Attachment is obsolete: true
Doh, mccr8's recent push added a new test file that wasn't in my move patch.

Take two:
This landed to mozilla-central, and after some drama and excitement, stuck:
(and ancestors)

Resolving fixed. \o/
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.