860029 - Standardize Modelines

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Sean Stangl [:sstangl]]

Attachment: patch

We occasionally fix up our modelines. This is one of those times.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Using tab-width 4 for emacs seems wrong. Tabs should always be 8 spaces long. Ideally this wouldn't matter since we should never have tab characters in the source, but we should still use the right value.

Comment 2

[Sean Stangl [:sstangl]]

(In reply to Bill McCloskey (:billm) from comment #1)
> Using tab-width 4 for emacs seems wrong. Tabs should always be 8 spaces
> long. Ideally this wouldn't matter since we should never have tab characters
> in the source, but we should still use the right value.

I'm not an emacs user, so I just copied the emacs template that ion/ and methodjit/ use. I'll change the emacs line back and change ion/ and methodjit/.

Comment 3

[Sean Stangl [:sstangl]]

Attachment: another patch

Same deal, but with a tab width of 8 for emacs.

Comment 4

[Nicholas Nethercote [inactive]]

FWIW, here's the Gecko standard from https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style

> /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> /* vim: set ts=8 sts=2 et sw=2 tw=80: */

So I'd suggest for SpiderMonkey we should use this:

> /* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
> /* vim: set ts=8 sts=4 et sw=4 tw=99: */

Comment 5

[Sean Stangl [:sstangl]]

(In reply to Nicholas Nethercote [:njn] from comment #4)
> > /* vim: set ts=8 sts=4 et sw=4 tw=99: */

This causes vim to insert 8 spaces for tabs, which is annoying, because I press tab to indent. The "sts=4" line also changes vim behavior by causing backspace to delete 4 spaces at a time -- I don't think it's a good idea to set that by default.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 735482 [details] [diff] [review]
another patch

Review of attachment 735482 [details] [diff] [review]:
-----------------------------------------------------------------

Why oh why oh why did you drag me into this?  ;-)

I can't quite tell which modeline gets used in gedit with the modelines plugin enabled, that affects how many spaces get added when you hit tab (or select a region and hit tab to indent it).  But these bits here, at least, don't trigger the eight-space indent thing, so I'm happy.

I didn't read the patch (no fear of that!), but I'd rather see the script/one-liner you used, than the actual patch itself.

Comment 7

[Nicholas Nethercote [inactive]]

> > > /* vim: set ts=8 sts=4 et sw=4 tw=99: */
> 
> This causes vim to insert 8 spaces for tabs, which is annoying, because I
> press tab to indent.

Let's be clear about distinguishing the pressing of the tab *key* from the treatment of tab *chars*.

The "sts=4" causes vim to insert 4 space chars when you hit the tab key, and the "ts=8" causes vim to display tab chars as taking up 8 columns... but you'll never insert tab chars because of the "et".  And the "sw=4" makes the '<' and '>' commands shift by 4.

> The "sts=4" line also changes vim behavior by causing
> backspace to delete 4 spaces at a time -- I don't think it's a good idea to
> set that by default.

It deletes up to 4 spaces -- enough to get back to the previous multiple-of-4 column, assuming it's all spaces back to that point.  I think it's an extremely good idea!  I've had this setting for years, and it's been the standard in Gecko for months.  It works fine.

Comment 8

[Sean Stangl [:sstangl]]

Okay, that sounds fine.

Comment 9

[Nicholas Nethercote [inactive]]

I'm happy to review an updated patch.

Comment 10

[Sean Stangl [:sstangl]]

Attachment: Gecko Standard Patch

An exciting patch for all ages.

Comment 11

[Nicholas Nethercote [inactive]]

Comment on attachment 736566 [details] [diff] [review]
Gecko Standard Patch

Review of attachment 736566 [details] [diff] [review]:
-----------------------------------------------------------------

Only having four lines of context was a good thing for this patch :)

Thanks for doing this.  I actually did skim through the whole patch!

::: js/public/Anchor.h
@@ +3,3 @@
>   *
>   * This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this

FWIW, in Gecko there's no blank line between the vim modeline and the license.  One less line of boilerplate at the top is probably a good thing, though if it's too hard to do I'll understand.

(That's for the MPL-licensed files...  for other files, such as the YARR ones, having a blank line before the license might be good.  I'll let you decide.)

::: js/src/assembler/moco/MocoStubs.h
@@ +14,5 @@
> +  public:
> +    JITCode(void* start, size_t size)
> +      : m_start(start), m_size(size)
> +    { }
> +    JITCode() { }

Well hello, what's this?  Let's not mix up non-modeline changes in this patch, please.

@@ +29,5 @@
> +  public:
> +    CodeBlock(JITCode& jc)
> +      : m_jitcode(jc)
> +    { }
> +    JITCode& getJITCode() { return m_jitcode; }

Ditto.

::: js/src/builtin/Module.cpp
@@ +2,5 @@
> + * vim: set ts=8 sts=4 et sw=4 tw=99:
> + *
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

There was no license in that file?  Lame.  Thanks for fixing.

::: js/src/devtools/jint/treesearch.py
@@ +2,4 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# vim: set ts=8 sts=4 et sw=4 tw=99: 

Whitespace at the end of the line.

::: js/src/ion/shared/IonFrames-shared.h
@@ +3,4 @@
>   *
> + * This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

You replaced the out-of-date license boilerplate, good.

::: js/src/jsapi-tests/testContexts.cpp
@@ +3,2 @@
>   */
>  /* This Source Code Form is subject to the terms of the Mozilla Public

In this case the modeline is in a separate comment from the license, instead of it all being in one.  It'd be good to put them all in one if it's not too hard.

(Likewise for lots of the other jsapi-tests files.)

::: js/src/jsfun.h
@@ +213,4 @@
>  
>      js::RawScript getOrCreateScript(JSContext *cx) {
>          JS_ASSERT(isInterpreted());
> +        JS_ASSERT(cx);

This shouldn't be in this patch, but I'll let it slide.

Comment 12

[Sean Stangl [:sstangl]]

(In reply to Nicholas Nethercote [:njn] from comment #11)
> ::: js/public/Anchor.h
> @@ +3,3 @@
> >   *
> >   * This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> 
> FWIW, in Gecko there's no blank line between the vim modeline and the
> license.  One less line of boilerplate at the top is probably a good thing,
> though if it's too hard to do I'll understand.

find -name *.cpp -or -name *.h -or -name *.c -or -name *.py -or -name *.js | xargs sed -i ':a;s/ \* *\n \* This Source Code Form/ \* This Source Code Form/g;N;ba'

Note that it takes quite a long time to run due to the flattening.

Comment 13

[Sean Stangl [:sstangl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf9771e1a28

Comment 14

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/6d92a2e39c77 - if you touch the build files, you have to copy-paste to the corresponding browser build files, or check-sync-dirs.py will burn the build (on the poor next victim's push, if you DONTBUILD the push where you break check-sync-dirs).

Comment 15

[Nicholas Nethercote [inactive]]

You missed some modelines in js/ipc/ and js/jsd/, but that doesn't matter much.

Comment 16

[Nicholas Nethercote [inactive]]

And DONTBUILD was bold for a patch touching so many files... I would have try server'd it myself :)

Comment 17

[Sean Stangl [:sstangl]]

"What's the worst that could happen with comment changes?", I naively thought to myself.

Comment 18

[Jeff Walden [:Waldo]]

Back in the MPL1 days someone broke the tree adding a name to the contributors list in a comment -- the name had Unicode characters or something in it, and the encoding or the Unicode-ness broke stuff.  :-)  I'm not sure if that failure's crazier than this one or not.  :-)

Comment 19

[Sean Stangl [:sstangl]]

Passed a tryserver run (https://tbpl.mozilla.org/?tree=Try&rev=25933b8e72a9, + build on all systems), so on to round 2.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a5a1dd3bd8ae

Comment 20

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b85daec2b90c for jsreftest orange

Comment 21

[Sean Stangl [:sstangl]]

https://tbpl.mozilla.org/?tree=Try&rev=ab52fdd9842d
https://hg.mozilla.org/integration/mozilla-inbound/rev/089c532f32d1

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/089c532f32d1