Closed Bug 1151541 Opened 7 years ago Closed 7 years ago

Fix mode lines in xpcom/


(Core :: XPCOM, defect)

Not set



Tracking Status
firefox40 --- fixed


(Reporter: mccr8, Assigned: mccr8)





(4 files)

The style fixups are great, but in at least some files, like nsObserverList, the mode lines have not been updated, so Emacs keeps wanting to indent where it shouldn't.  I'll try to get to this after the dust settles a little from this observer service crash so I don't bitrot myself.
Nathan, do you have an opinion on the style that mashes the mode line into the same comment as the MPL vs. not?  (Like in xpcom/ds/nsVariant.cpp)
Flags: needinfo?(nfroyd)
I'm inclined towards separate comments for the MPL and mode lines.  I think it makes sed-style substitutions slightly easier to write, though this applies more to the MPL than the mode lines, I guess.
Flags: needinfo?(nfroyd)
I wrote a probably-overengineered script to fix mode lines.  It attempts to figure out if the file is 2-space or 4-space indented based on the ratio of lines indented by an amount divisible by 2 vs 4, and quits with an error for those files, so you have to add them to a blacklist.  It also makes sure that the MPL2 at the start of the file looks okay.

I ignored a few directories that look like they haven't had their style fixed yet:

Individual files that were blacklisted:
    # Not a regular source file.
    # Imported.
    # Public domain instead of MPL.
    # Odd tiny header.
    # Partially or fully 4-space indented.
    'xpcom/tests/gtest/TestXPIDLString.cpp', # Also missing license header.

Files where I ignored indentation checking failures. I manually verified that these looked 2 space indented:
    # Mostly function decls, so there are few normal lines.
    # Formatting is a little weird, but looks 2-space indented to me.
Summary: Fix mode lines in XPCOM → Fix mode lines in xpcom/
My script incidentally detects missing MPL2 headers.  The missing header in StaticMutex just looks like an oversight.  For stub_test, other files in that directory have the MPL header so it seems reasonable to add.  In general, the tests are not consistent about including a license, but I mostly just blacklisted them.
Attachment #8589800 - Flags: review?(nfroyd)
This patch is entirely generated by my script.
Comment on attachment 8589800 [details] [diff] [review]
part 1 - Add MPL2 to StaticMutex.h and stub_test.cpp.

Review of attachment 8589800 [details] [diff] [review]:

Attachment #8589800 - Flags: review?(nfroyd) → review+
Comment on attachment 8589801 [details] [diff] [review]
part 2 - Fix mode lines in xpcom/.

It builds and runs locally.  The caveats in comment 3 may be of interest when reviewing.
Attachment #8589801 - Flags: review?(nfroyd)
Comment on attachment 8589801 [details] [diff] [review]
part 2 - Fix mode lines in xpcom/.

Review of attachment 8589801 [details] [diff] [review]:


For those files that got changed to tab-width: 8, I wonder if they have any tabs in them?
Attachment #8589801 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #9)
> For those files that got changed to tab-width: 8, I wonder if they have any
> tabs in them?

Good point.  I can see if there are any files with leading tabs that are getting modified.
Thanks to your suggestion, I added a mode to check leading tabs in files we modify the mode line for. It found a few minor things.

I also did a random style fixup in one of the tabby parts of the code.
Attachment #8590354 - Flags: review?(nfroyd)
Comment on attachment 8590354 [details] [diff] [review]
part 3 - Fix leading tabs in xpcom/.

Review of attachment 8590354 [details] [diff] [review]:

My other maintainer hat says "WHITESPACE POLICE".
Attachment #8590354 - Flags: review?(nfroyd) → review+
See Also: → 1412119
You need to log in before you can comment on or make changes to this bug.