Closed
Bug 1151541
Opened 9 years ago
Closed 9 years ago
Fix mode lines in xpcom/
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: mccr8, Assigned: mccr8)
References
()
Details
Attachments
(4 files)
1.22 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
77.38 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
8.52 KB,
text/plain
|
Details | |
7.55 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•9 years ago
|
||
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)
Comment 2•9 years ago
|
||
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)
Assignee | ||
Comment 3•9 years ago
|
||
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: xpcom/tests xpcom/tests/static-checker xpcom/tests/windows xpcom/typelib/xpt xpcom/typelib/xpt/tests xpcom/tests xpcom/reflect/* Individual files that were blacklisted: # Not a regular source file. 'xpcom/base/ErrorList.h', # Imported. 'xpcom/base/pure.h', 'xpcom/build/mach_override.h', 'xpcom/glue/nsQuickSort.cpp', # Public domain instead of MPL. 'xpcom/glue/tests/gtest/TestFileUtils.cpp', # Odd tiny header. 'xpcom/io/crc32c.h', # Partially or fully 4-space indented. 'xpcom/tests/gtest/TestSynchronization.cpp', 'xpcom/tests/gtest/TestTimeStamp.cpp', 'xpcom/tests/gtest/TestXPIDLString.cpp', # Also missing license header. 'xpcom/base/nsAgg.h', 'xpcom/components/ModuleUtils.h', 'xpcom/windbgdlg/windbgdlg.cpp', 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. 'xpcom/io/nsStreamUtils.h', 'xpcom/string/nsReadableUtils.h', 'xpcom/build/nsXULAppAPI.h', # Formatting is a little weird, but looks 2-space indented to me. 'xpcom/tests/gtest/TestThreads.cpp', 'xpcom/tests/gtest/TestUTF.cpp',
Assignee | ||
Updated•9 years ago
|
Summary: Fix mode lines in XPCOM → Fix mode lines in xpcom/
Assignee | ||
Comment 4•9 years ago
|
||
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)
Assignee | ||
Comment 5•9 years ago
|
||
This patch is entirely generated by my script.
Assignee | ||
Comment 6•9 years ago
|
||
Comment 7•9 years ago
|
||
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]: ----------------------------------------------------------------- Sure.
Attachment #8589800 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 8•9 years ago
|
||
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 9•9 years ago
|
||
Comment on attachment 8589801 [details] [diff] [review] part 2 - Fix mode lines in xpcom/. Review of attachment 8589801 [details] [diff] [review]: ----------------------------------------------------------------- WFM. 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+
Assignee | ||
Comment 10•9 years ago
|
||
(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.
Assignee | ||
Comment 11•9 years ago
|
||
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 12•9 years ago
|
||
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+
Assignee | ||
Comment 13•9 years ago
|
||
Thanks for the reviews. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/f9d2b45fca62 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/2328731ef453 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6ba780f5190e
Comment 14•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f9d2b45fca62 https://hg.mozilla.org/mozilla-central/rev/2328731ef453 https://hg.mozilla.org/mozilla-central/rev/6ba780f5190e
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•