Closed
Bug 688012
Opened 13 years ago
Closed 13 years ago
Fix the most egregious xpconnect formatting abnormalities
Categories
(Core :: XPConnect, defect)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(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.
Assignee | ||
Comment 1•13 years ago
|
||
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
Comment 2•13 years ago
|
||
Did I see something about js/xpconnect on irc? ;-)
Assignee | ||
Comment 3•13 years ago
|
||
(In reply to Luke Wagner [:luke] from comment #2) > Did I see something about js/xpconnect on irc? ;-) Sure, I can do that. :-)
Assignee | ||
Comment 4•13 years ago
|
||
(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
Assignee | ||
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
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.
Attachment #566865 -
Flags: review?(mrbkap)
Comment 7•13 years ago
|
||
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+
Assignee | ||
Comment 8•13 years ago
|
||
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.
Attachment #566864 -
Attachment is obsolete: true
Assignee | ||
Comment 9•13 years ago
|
||
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
Assignee | ||
Comment 10•13 years ago
|
||
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/
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•