On Fri, Oct 19, 2018 at 6:40 AM Kent Williams <kwilli...@leepfrog.com>
wrote:
> I understand the urge to have consistent formatting, and that ideas
> about the best way to format evolve over time.
>
> Here's the A#1 reason this is NOT necessarily a good idea: It obscures
> the real changes when you compare versions.
>
> If you want to change formatting standards, knock yourselves out. But I
> would propose you think about adding another tool to the arsenal:
>
> Since you are going to automate the formatting change also implement a
> filter -- I believe you can install it as a git or mercurial or whatever
> extension -- that will reformat the old version of the code before
> comparing to the new version, so that the formatting changes are taken
> out of the diff output.
>
The problem with this approach is that the code ends up inconsistent until
every line of code has been modified for some other reason. Inconsistent
style distracts readers and confuses patch authors, which is why we haven't
done it in the past.
It's worth noting that searchfox is very good at jumping past reformats
when looking up blame.
>
> I generally do diffs with --ignore-all-space; this would be a stronger
> option, that reformats the old code to match the new code's format
> before diffing.
>
> On 09/05/2018 08:23 AM, Ehsan Akhgari wrote:
> > On Tue, Sep 4, 2018 at 12:49 PM Bobby Holley <bobbyhol...@gmail.com>
> wrote:
> >
> >> +1
> >>
> >> The one downside of doing this now is that we'll eventually do another
> bulk
> >> reformat of all of mozilla-central once we settle on a clang-format
> >> version+config whose output we're happy with. So if that were to happen
> >> very soon (unlikely), doing piecemeal handling of braces now would
> result
> >> in extra effort and blame churn that could have been avoided. But since
> the
> >> clang-format work will probably take a while, switching now will allow
> new
> >> code to more-closely match long-term style, and allow everyone to get
> >> comfortable with it.
> >>
> >
> > It's not quite clear when the clang-format conversion will happen yet and
> > what it will look like yet, but I can tell you that it isn't *imminent*
> (as
> > in, not in the next couple of weeks for example), in case that helps
> making
> > the decision here.
> >
> > Cheers,
> > Ehsan
> >
> >
> >> bholley
> >>
> >> On Tue, Sep 4, 2018 at 7:41 AM Jan de Mooij <jdemo...@mozilla.com>
> wrote:
> >>
> >>> Hi all,
> >>>
> >>> I'd like to propose we change the SpiderMonkey coding style to always
> >> brace
> >>> if/for/while statements.
> >>>
> >>> It matches Gecko's coding style and in the past there has been
> agreement
> >> to
> >>> unify our coding styles as much as possible. Adding unnecessary braces
> >>> often results in style nits when Gecko hackers write SpiderMonkey
> patches
> >>> (or similarly, missing braces in Gecko code). The situation is worse in
> >>> XPConnect where both styles are used. Also, when to use {} can be
> pretty
> >>> confusing for people new to SpiderMonkey.
> >>>
> >>> The downside is that it's more verbose, especially for a file like
> >>> BytecodeEmitter.cpp that has tons of unbraced if-statements (it will
> add
> >>> about 900-1000 lines to that file).
> >>>
> >>> The conversion is pretty easy to automate - while waiting for feedback
> >> and
> >>> Try results today I wrote a simple script to do this, here's the diff
> for
> >>> BytecodeEmitter.cpp: https://dpaste.de/HKvM -- I still have to review
> >> the
> >>> changes more closely (and probably compare the generated object files)
> >> but
> >>> overall it looks reasonable.
> >>>
> >>> Thoughts?
> >>>
> >>> Thanks,
> >>> Jan
> >>> _______________________________________________
> >>> dev-tech-js-engine-internals mailing list
> >>> dev-tech-js-engine-internals@lists.mozilla.org
> >>> https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
> >>>
> >> _______________________________________________
> >> dev-tech-js-engine-internals mailing list
> >> dev-tech-js-engine-internals@lists.mozilla.org
> >> https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
> >>
> >
> >
>
> _______________________________________________
> dev-tech-js-engine-internals mailing list
> dev-tech-js-engine-internals@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
>
_______________________________________________
dev-tech-js-engine-internals mailing list
dev-tech-js-engine-internals@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-tech-js-engine-internals
