On 12/19/2013, 9:36 PM, Brian Smith wrote:
On Thu, Dec 19, 2013 at 6:13 PM, Ehsan Akhgari <ehsan.akhg...@gmail.com
<mailto:ehsan.akhg...@gmail.com>> wrote:
But to address the main point of this paragraph, what's wrong with
having *one* style that *everybody* follows? I can't tell if you
have something against that, or if you just care about a small
subset of the tree and are happy with the status quo in that subset.
I've asked people in PSM land to follow the Mozilla Coding Style for new
code, except for modifications to pre-existing code. When a group of new
people started working on PSM, I took some time to make a widespread
change throughout many parts of PSM to make it more consistent,
coding-style-wise. However, there are a bunch of rules that I did not
enforce as part of that change. I've been tempted to make another mass
change to do so, and I am open to other people submitting patches in my
module to make the code more consistent with the Mozilla coding style.
As a Necko peer, I would welcome such changes in Necko too. However, it
would be great to agree on what changes are going to be done, before a
large amount of effort is spent doing them.
Thanks a lot for doing this!
I don't think that everybody is as agreeable as me though. When I've
been asked to review code in other modules, my attempts to get people to
follow the Mozilla coding style, instead of the module peers'/owners'
style, received a lot of pushback. WebRTC comes to mind, though I think
the pushback was probably more over concern for delaying the landing of
the feature over concern about style.
I have been rejected enough number of times suggesting style fixes or
that module owners follow our coding style that I've basically stopped
trying (not because I'm happy with the result of my attempts.)
Personally, there are a couple of things I don't like about
moz-style (though revisions to the central style guide at least
have made it better than it used to be), but instead of
bikeshedding the central style guide, we just do our own thing
in the code we're responsible for.
That is not very helpful. If there is something in the mozilla
style guide that you think is wrong and needs to change, *please*
bring it up. If you're right, you'll be benefiting everyone. And
if it's just a matter of taste, perhaps you could sacrifice your
preferences in the interest of the greater good?
In PSM, we created some scoped pointer wrapper types around NSS data
structures (ScopedNSSTypes.h), which are based on the MFBT scoped
pointers. And, consequently, PSM has standardized on MFBT smart pointers
throughout the module (there should be nsRefPtr in PSM, only RefPtr, for
example). Yet, most code in Gecko is based on the nsCOMPtr-like smart
pointers (nsAutoPtr, nsRefPtr). I don't know how big of a deal this is,
but this is the type of thing that would need to be resolved to have a
consistent style throughout Gecko.
Choosing whether you want nsRefPtr or RefPtr is more than just a matter
of style as they provide different functionality (already_AddRefed comes
to mind.)
> It's a decent amount of work to restyle the modules well
That's actually not true. There are tools which are very good at
doing this work once we agree that it should be done.
Color me skeptical. I wouldn't want somebody to reformat the code in the
modules I have responsibility for without reviewing the changes. And,
reviewing tens of thousands of lines of changes is a lot of work.
You would be surprised how well clang-format works. (Note that it
mostly tries to match the current style of the file, so it fails
miserably on files which have variying different styles -- which
includes some of gecko!, but it can be modified to not use heuristics on
the current file rather easily.)
I don't think that anybody is suggesting that we come up with a set
of style guides and carve them into stone and never consider
anything otherwise. But then again debating where the * in a
pointer notation ends up with every week isn't the best use of
everybody's time. If and when someone finds something wrong in the
style guideline they can bring it up and get the style modified if
they have a good point. Note that this is quite doable, as evidence
of other projects which do this well shows.
If somebody submitted a patch to fix the "*" issue throughout PSM, I
would r+ it, though I don't look forward to spending the time to do it,
especially considering the issue of bitrot. (Please do not write such a
patch before the end of January; it wouldn't get r+d before then, due to
the bitrot issue with pending work.)
You may soon find such a patch waiting your review. ;-)
I suppose my counter-question is 'How does standardizing styles
across modules help us?' In my experience, reviewing (or being
reviewed) for style takes almost no time.
I agree. With two exceptions, everything style-related related seems to
be insignificant regarding how much time I spend on stuff. I just make
my code look like the code around it, and if the reviewer complains
about style issues I generally just do whatever the reviewer wants so I
don't need to argue back-and-forth. Very simple.
However, there are two issues that are non-trivial distractions from
real work:
1. Many parts of NSS use tabs instead of spaces. AFAICT, this is an
issue for which the idea of fixing things is more-or-less agreed upon.
But, no time to actually do it.
2. Not everybody succeeds at making their new code look like the code
around it (or, in some cases, like any other code on Earth). I (and
others) waste a large amount of time during code reviews pointing out
style nits. If there were a tool that people were required to run to
self-review their code before asking for review, and that tool required
work to make our code more consistent in order for it to work correctly,
I would be more enthusiastic about taking time to deal with the
inconvenience of reformatting the parts of the codebase I work on (NSPR
and NSS aside, since other than replacing tabs with spaces, it is
unlikely to get agreement from the other peers/owners to make wholesale
style changes in those modules.)
In bug 875605 BenWa was working on something like that.
Cheers,
Ehsan
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
