Coding style for C++ enums

[Kartikaya Gupta]

Is there a recommendation for what enum values in C++ code should be
styled as? The coding style doesn't say and we use a variety of things
in existing code, so I was wondering if we should settle on something
for new enums being added to the code, and update the style guide
accordingly.

enum OptionA {
ALL_CAPS_VALUES,
THIS_IS_LIKE_SHOUTING
};

enum OptionB {
SentenceCaseValues,
ThisMightBeConfusedWithOtherThings
};

enum OptionC {
eSentenceCaseValues,
eThisSeemsPopular
};

Others?

kats

[Birunthan Mohanathas]

On 8 April 2016 at 18:10, Kartikaya Gupta <kgu...@mozilla.com> wrote:
> Others?

enum class OptionD {
SentenceCaseValues,
ThisWontBeConfusedWithOtherThings
// ... because you need to use OptionD::ThisWontBeConfusedWithOtherThings
};

[smaug]

Hmm, I thought we had eFoo in coding style, but don't see it there.
Personally I don't like all caps, since those looks like macros, and
CamelCase looks like type name.
So I'd prefer e-prefix, and for example all the event names moved to
use that syntax recently (well, the names used to be #define SOME_EVENT).
That made, IMO, the code easier to read.


-Olli






>
> Others?
>
> kats
>

[Nick Fitzgerald]

Strong +1 for enum classes. That way we don't need any more nasty prefixing
and can
​instead ​
rely on the compiler
​​
and type system.

[smaug]

enum classes are great, but I'd still use prefix for the values. Otherwise the values look like types - nested classes or such.
(Keeping my reviewer's hat on, so thinking about readability here.)



-Olli

[Jeff Gilbert]

Strong preference against eFoo, here. :)

Just use enum classes.

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Mats Palmgren]

On 2016-04-08 23:03, Jeff Gilbert wrote:
> Strong preference against eFoo, here. :)

Strong preference *for* eFoo, here. :)

If I see Bar::Foo anywhere in code I'm not familiar with, my brain
is likely to first parse that as a type before realizing that, hmm
that doesn't make sense in an expression context, and then I will
likely have to lookup what that silly Bar thing is just to be sure.

eFoo is unambiguous and utterly clear.

/Mats

[Jonas Sicking]

On Fri, Apr 8, 2016 at 2:03 PM, Jeff Gilbert <jgil...@mozilla.com> wrote:
> Strong preference against eFoo, here. :)

Would you mind saying why?

/ Jonas

[Jeff Gilbert]

It's noisy in code you *do* understand, which is the bulk of the code
we should be dealing with the majority of the time.

I do not parse this initially as a type because that generally doesn't
make sense given context.
Generally the names involved are also unlikely to be types, based on name alone.

It would also add to our already-pungent code smell.

[Aryeh Gregor]

On Fri, Apr 8, 2016 at 8:35 PM, smaug <sm...@welho.com> wrote:
> enum classes are great, but I'd still use prefix for the values. Otherwise the values look like types - nested classes or such.
> (Keeping my reviewer's hat on, so thinking about readability here.)

In some cases I think it's extremely clear, like this:

enum class Change { minus, plus };

whose sole use is as a function parameter (to avoid a boolean
parameter), like so:

ChangeIndentation(*curNode->AsElement(), Change::plus);

In cases where it might be mistaken for a class, it might make more
sense to prefix the enum class name with "E". I don't think this
should be required as a general policy, though, because for some enums
it might be clear enough without it.

[smaug]

Indeed in your case it is totally unclear whether one is passing a function pointer or enum value as param.
If the value was prefixed with e, it would be clear.

Consistency helps with reviewing (and in general code readability) a lot, so the less we have optional coding style rules, or
context dependent rules, the better.

[Chris Peterson]

On 4/8/16 8:10 AM, Kartikaya Gupta wrote:
> Is there a recommendation for what enum values in C++ code should be
> styled as? The coding style doesn't say and we use a variety of things
> in existing code, so I was wondering if we should settle on something
> for new enums being added to the code, and update the style guide
> accordingly.

FWIW, Google's C++ Style Guide dictates that enums in new code use
kPrefixedNames. SHOUTY_CAPS are allowed in older code.

https://google.github.io/styleguide/cppguide.html#Enumerator_Names

[Xidorn Quan]

On Mon, Apr 11, 2016 at 5:17 PM, Chris Peterson <cpet...@mozilla.com>
wrote:

That's for enum. But for enum class, that probably doesn't make that much
sense.

In terms of readability, I probably agree with prefixing the items with "e"
so that people do know it is an enum constant. An additional "e" isn't
ridiculous long, so this tradeoff should be considered acceptable.

- Xidorn

[Mats Palmgren]

On 2016-04-08 23:03, Jeff Gilbert wrote:

> Strong preference against eFoo, here. :)

[Seth Fowler]

I agree in the extreme with Jeff here. Foo::Bar, in an expression context, cannot be anything other than a value. For me, at least, I have never noticed this adding any overhead in reviews - it’s immediately obvious what’s happening. I suspect that if it *does* add overhead for you, you’ll find that overhead disappearing as you get more used to reading such code. Adding a prefix like eFoo or kFoo is just visual noise that is actively harmful, because in combination with our 80-character limit for lines, it pushes us into more awkward code formatting that makes things harder to read in a very real sense. Awkward line wrapping like that *definitely* makes it harder for me to review code in a very real way.

If we want to have some additional visual indication that we’re looking at an enum value, I suggest just using Foo::BAR, which is already in use in some parts of the code and which I think is extremely unambiguous.

Thanks,
- Seth

> On Apr 8, 2016, at 3:17 PM, Jeff Gilbert <jgil...@mozilla.com> wrote:
>
> It's noisy in code you *do* understand, which is the bulk of the code
> we should be dealing with the majority of the time.
>
> I do not parse this initially as a type because that generally doesn't
> make sense given context.
> Generally the names involved are also unlikely to be types, based on name alone.
>
> It would also add to our already-pungent code smell.
>
> On Fri, Apr 8, 2016 at 3:03 PM, Mats Palmgren <ma...@mozilla.com> wrote:

[Kartikaya Gupta]

Based on all the feedback so far I think the best compromise is to use
enum classes with the "e" prefix on the values. As was mentioned, the
"e" prefix is useful to distinguish between enum values and function
pointer passing at call sites, but is a small enough prefix that it
shouldn't be too much of a burden.

I realize not everybody is going to be happy with this but I don't
want this discussion to drag on to the point where it's a net loss of
productivity no matter what we end up doing. I'll update the style
guide.

Thanks all!

kats

[Milan Sreckovic]

Maybe there was a reason we didn’t have this in the style guide as a single choice.
—
- Milan

> On Apr 11, 2016, at 11:00 , Kartikaya Gupta <kgu...@mozilla.com> wrote:
>
> Based on all the feedback so far I think the best compromise is to use
> enum classes with the "e" prefix on the values. As was mentioned, the
> "e" prefix is useful to distinguish between enum values and function
> pointer passing at call sites, but is a small enough prefix that it
> shouldn't be too much of a burden.
>
> I realize not everybody is going to be happy with this but I don't
> want this discussion to drag on to the point where it's a net loss of
> productivity no matter what we end up doing. I'll update the style
> guide.
>
> Thanks all!
>
> kats
>
>
> On Sun, Apr 10, 2016 at 11:43 AM, smaug <sm...@welho.com> wrote:

[smaug]

On 04/11/2016 09:01 PM, Milan Sreckovic wrote:
> Maybe there was a reason we didn’t have this in the style guide as a single choice.

My guess is that, even though there has been the Mozilla coding style forever,
we haven't traditionally been too strict about following it, so no one cared too much.
But since we're now trying to be more consistent, it does make sense to have some rule for this case too.
That is needed also in case we want to use some automatic tool to reformat larger parts of the code to follow the rules.


-Olli

[Seth Fowler]

> All caps actually makes long names *longer* because you now need to
> add underscores to separate words, rather than using sentence case.
> For example eSentenceCase is the same length as SENTENCE_CASE, but if
> you tack on another word the "e" prefix is shorter. Presumably this
> line-wrapping issue is more pronounced with long names than short
> names, so this is actually an argument against all caps.
>

That's a great point. My focus is mostly on ensuring that the single word
cases (.e.g Color::BLUE), which in my experience are extremely common if
not the vast majority, don't get penalized. I actually think the issue
becomes less pronounced with very long names, because at a certain point
they're so long that extra characters don't matter much anyway in terms of
code formatting impact.

- Seth

[Jeff Gilbert]

I'm not sure how this is compromise. We were already supposed to use
enum classes with new code.

Every additional glyph incurs mental load. Code should instead be
written so these things are not necessary to explicitly state. *That*
is the code we should be trying to write. In well-written code,
superfluous warts detract from readability.

At this point, it feels a lot like I could propose full hungarian
notation to this list piecemeal, and there would be ongoing
'compromises' to add it one step at a time. After all, who wants to
trace back to the variable declaration when the variable name can tell
us what type it is?


I propose that if you're in a part of the codebase where you can't
tell if it's an enum or a function pointer (regardless of the fact
that the compiler can't confuse these), you should do more looking
around before working on it, much less reviewing changes to the code.
If you can't tell if it's an enum, you're not going to be able to
effectively make changes, so buckle up and do some looking around.


To go completely off the rails, I no longer even believe that all of
Gecko should have a single style. I think the whole attempt is
increasingly a distraction vs the alternative, and I say this as
someone who writes and reviews under at least three differently-styled
code areas. The JS engine will, as ever, remain distinct, as will
third-party code, and also all non-actively maintained files. (most of
our codebase)

Yes, a formatter could magic-wand us to most of the superficial
aspects of a single code style, but we'd then need to backfill
readability into some of the trickier bits. Sometimes we need to step
outside the prevailing style to keep something reasonably readable.

But we /already have/ a process for keeping things readable: Code reviews.
And if that fails: Module ownership.

I'm not sure why we hold centralization in such high regard when it
comes to code style. And, if we are continuing to centralize this,
we're going to need to have an actual process for making decisions
here.

I suspect the goal is to have an oracle we can ask how we should
format code in order to be readable. However, we can't even agree on
it, so the right answer often is indeterminate. However, people within
their modules have a better idea of what works in each case.

I do think we should have some high-level style guidance, but I think
we're increasingly micromanaging style. Let code review take care of
the specifics here. If it's not readable without prefixes, definitely
consider adding prefixes. Otherwise don't require it.


FWIW, I do have a preference for k prefix instead of e prefix, (if we
must have a required prefix) since they are in the same 'constants'
category. 'k' I can be persuaded the usefulness of for enums. 'e', not
so much.

On Mon, Apr 11, 2016 at 8:00 AM, Kartikaya Gupta <kgu...@mozilla.com> wrote:
> Based on all the feedback so far I think the best compromise is to use
> enum classes with the "e" prefix on the values. As was mentioned, the
> "e" prefix is useful to distinguish between enum values and function
> pointer passing at call sites, but is a small enough prefix that it
> shouldn't be too much of a burden.
>
> I realize not everybody is going to be happy with this but I don't
> want this discussion to drag on to the point where it's a net loss of
> productivity no matter what we end up doing. I'll update the style
> guide.
>
> Thanks all!
>
> kats
>
>
> On Sun, Apr 10, 2016 at 11:43 AM, smaug <sm...@welho.com> wrote:

[Seth Fowler]

I'd honestly prefer to see this discussion drag on a little longer. The
original email was on Friday, so given that most participants on this list
aren't actively debating C++ coding style on the weekend, we've actually
had less than one full working day of discussion on this issue so far.

Let me suggest again (especially since I'm not sure my previous email
reached this list yet) that rather than compromise on the "e" prefix, we
compromise on all caps for values, which is just as readable without
placing even more pressure on our 80-character line length restriction.

Thanks,
- Seth

On Mon, Apr 11, 2016 at 8:00 AM, Kartikaya Gupta <kgu...@mozilla.com> wrote:

> Based on all the feedback so far I think the best compromise is to use
> enum classes with the "e" prefix on the values. As was mentioned, the
> "e" prefix is useful to distinguish between enum values and function
> pointer passing at call sites, but is a small enough prefix that it
> shouldn't be too much of a burden.
>
> I realize not everybody is going to be happy with this but I don't
> want this discussion to drag on to the point where it's a net loss of
> productivity no matter what we end up doing. I'll update the style
> guide.
>
> Thanks all!
>
> kats
>
>
> On Sun, Apr 10, 2016 at 11:43 AM, smaug <sm...@welho.com> wrote:

[smaug]

On 04/12/2016 12:12 AM, Jeff Gilbert wrote:
> I'm not sure how this is compromise. We were already supposed to use
> enum classes with new code.
>
> Every additional glyph incurs mental load.

But it reduces the mental load from deciding from the context what kind of thing
one is dealing with (is it type, value, function name...)

> Code should instead be
> written so these things are not necessary to explicitly state. *That*
> is the code we should be trying to write. In well-written code,
> superfluous warts detract from readability.
>
> At this point, it feels a lot like I could propose full hungarian
> notation to this list piecemeal, and there would be ongoing
> 'compromises' to add it one step at a time. After all, who wants to
> trace back to the variable declaration when the variable name can tell
> us what type it is?

Hungarian notation is more about the type, less about the scope or such.
But sure, there is the risk to add too many rules. So far we've focused on the
scope and kind, and that is where I think we'll find the rules for easiest to read code.
And I consider our coding style rules to be rather simple.

>
>
> I propose that if you're in a part of the codebase where you can't
> tell if it's an enum or a function pointer (regardless of the fact
> that the compiler can't confuse these), you should do more looking
> around before working on it, much less reviewing changes to the code.
> If you can't tell if it's an enum, you're not going to be able to
> effectively make changes, so buckle up and do some looking around.

Perhaps this is purely subjective matter then, but for me all the prefixes we use
ease reviewing. No need to look around what I'm dealing with. And it doesn't matter whether I'm familiar
or not with that particular code, it is always just faster to read the code when the code itself
tells me clearly what kind of thing it is.
(this is a bit similar to why I don't like use of 'auto', it tends to just hide useful information from the reader.)

>
> To go completely off the rails, I no longer even believe that all of
> Gecko should have a single style.

That is possibly true. Say, JS eng devs seem to insist they need to keep their style, and fine,
perhaps it would decrease their productivity if JS eng was converted to more common coding style.
But, JS engine is rather separate module, so it using separate style is manageable.
It would be weirder, and make reviewers' life harder, if say DOM and layout would start to use totally different styles.

> I think the whole attempt is
> increasingly a distraction vs the alternative, and I say this as
> someone who writes and reviews under at least three differently-styled
> code areas. The JS engine will, as ever, remain distinct, as will
> third-party code, and also all non-actively maintained files. (most of
> our codebase)

Most?

>
> Yes, a formatter could magic-wand us to most of the superficial
> aspects of a single code style, but we'd then need to backfill
> readability into some of the trickier bits. Sometimes we need to step
> outside the prevailing style to keep something reasonably readable.
>
> But we /already have/ a process for keeping things readable: Code reviews.
> And if that fails: Module ownership.

Well, it is a tiny bit frustrating to spend reviewing time for coding style issues.
And when one is spending most of the time doing reviews, these things do matter, at least to me.

>
> I'm not sure why we hold centralization in such high regard when it
> comes to code style. And, if we are continuing to centralize this,
> we're going to need to have an actual process for making decisions
> here.
>
> I suspect the goal is to have an oracle we can ask how we should
> format code in order to be readable. However, we can't even agree on
> it, so the right answer often is indeterminate. However, people within
> their modules have a better idea of what works in each case.

That is what we need to stop doing. We need to agree on something, and just use it. It often doesn't matter
too much what we end up agreeing, but just that what we do agree is something we use then consistently.
Mozilla has had coding style rules for ages (13+ years) but for some reason those haven't been followed too well.

>
> I do think we should have some high-level style guidance, but I think
> we're increasingly micromanaging style. Let code review take care of
> the specifics here. If it's not readable without prefixes, definitely
> consider adding prefixes. Otherwise don't require it.
>
>
> FWIW, I do have a preference for k prefix instead of e prefix, (if we
> must have a required prefix) since they are in the same 'constants'
> category. 'k' I can be persuaded the usefulness of for enums. 'e', not
> so much.

I could live with 'k', though, don't understand why to not use 'e' when it gives more
information to the reader.



Bear with me. I'm starting to feel like a grumpy old reviewer.


-Olli

[Mats Palmgren]

On 2016-04-11 19:47, Seth Fowler wrote:
> Let me suggest again (especially since I'm not sure my previous email
> reached this list yet) that rather than compromise on the "e" prefix, we
> compromise on all caps for values, which is just as readable without
> placing even more pressure on our 80-character line length restriction.

I'm opposed to that, for the following reasons:
1. dropping the 'e' only makes the line shorter for single word values,
and makes it *longer* for THREE_WORD_VALUES
2. many people read all-caps as SCREAMING
3. the claim that a single 'e' makes it harder to keep lines under 80
columns to any significant degree is extremely weak
4. the 'e' prefix is already the de-facto standard and widely used in
our code base

I do agree with you though that our 80-column rule produces awkward
line-wrapping that makes it hard to read, write and review code.
The solution to /that/ problem is to relax that rule so that using
longer lines is allowed when code readability so demands. The root
cause for this is usually our use of VeryLongAndDescriptiveNames
(which is a good thing). We shouldn't sacrifice descriptive names,
nor code readability, for this archaic 80-column rule.

So, my counter-proposal is this:
Try to keep lines to 80 columns or less, as a rule of thumb.
Use up to 100 columns (hard limit) when code readability so demands.

Thanks,
Mats

[Bobby Holley]

On Mon, Apr 11, 2016 at 2:12 PM, Jeff Gilbert <jgil...@mozilla.com> wrote:

> I'm not sure how this is compromise. We were already supposed to use
> enum classes with new code.
>

> Every additional glyph incurs mental load. Code should instead be

> written so these things are not necessary to explicitly state. *That*
> is the code we should be trying to write. In well-written code,
> superfluous warts detract from readability.
>
> At this point, it feels a lot like I could propose full hungarian
> notation to this list piecemeal, and there would be ongoing
> 'compromises' to add it one step at a time. After all, who wants to
> trace back to the variable declaration when the variable name can tell
> us what type it is?
>
>

> I propose that if you're in a part of the codebase where you can't
> tell if it's an enum or a function pointer (regardless of the fact
> that the compiler can't confuse these), you should do more looking
> around before working on it, much less reviewing changes to the code.
> If you can't tell if it's an enum, you're not going to be able to
> effectively make changes, so buckle up and do some looking around.
>

I strongly disagree with this statement and the sentiment behind it. The
ability for certain people to be able to quickly review changes in code
that they don't have paged into working memory is vital to the survival of
Gecko.

Mozilla is very bad at distributing review loads. There are literally a
handful of people holding the ship together (and smaug is one of them). It
is not a fun job, and the bus-factor is frighteningly low. This means that
we should absolutely give more weight to the preferences of top reviewers
because:
(a) We owe it to them, and
(b) We want to make it easier for other people to join their ranks.

To go completely off the rails, I no longer even believe that all of
> Gecko should have a single style.

Yes, that is off the rails in this context.

> I think the whole attempt is
> increasingly a distraction vs the alternative, and I say this as
> someone who writes and reviews under at least three differently-styled
> code areas. The JS engine will, as ever, remain distinct

Jason agreed to converge SM style with Gecko.

> , as will
> third-party code, and also all non-actively maintained files. (most of
> our codebase)
>

> Yes, a formatter could magic-wand us to most of the superficial
> aspects of a single code style, but we'd then need to backfill
> readability into some of the trickier bits. Sometimes we need to step
> outside the prevailing style to keep something reasonably readable.
>
> But we /already have/ a process for keeping things readable: Code reviews.
> And if that fails: Module ownership.
>

> I'm not sure why we hold centralization in such high regard when it
> comes to code style. And, if we are continuing to centralize this,
> we're going to need to have an actual process for making decisions
> here.
>
> I suspect the goal is to have an oracle we can ask how we should
> format code in order to be readable. However, we can't even agree on
> it, so the right answer often is indeterminate. However, people within
> their modules have a better idea of what works in each case.
>

> I do think we should have some high-level style guidance, but I think
> we're increasingly micromanaging style. Let code review take care of
> the specifics here. If it's not readable without prefixes, definitely
> consider adding prefixes. Otherwise don't require it.
>
>
> FWIW, I do have a preference for k prefix instead of e prefix, (if we
> must have a required prefix) since they are in the same 'constants'
> category. 'k' I can be persuaded the usefulness of for enums. 'e', not
> so much.
>
>

> On Mon, Apr 11, 2016 at 8:00 AM, Kartikaya Gupta <kgu...@mozilla.com>
> wrote:
> > Based on all the feedback so far I think the best compromise is to use
> > enum classes with the "e" prefix on the values. As was mentioned, the
> > "e" prefix is useful to distinguish between enum values and function
> > pointer passing at call sites, but is a small enough prefix that it
> > shouldn't be too much of a burden.
> >
> > I realize not everybody is going to be happy with this but I don't
> > want this discussion to drag on to the point where it's a net loss of
> > productivity no matter what we end up doing. I'll update the style
> > guide.
> >
> > Thanks all!
> >
> > kats
> >
> >
> > On Sun, Apr 10, 2016 at 11:43 AM, smaug <sm...@welho.com> wrote:

[Kartikaya Gupta]

On Mon, Apr 11, 2016 at 1:46 PM, Seth Fowler <se...@mozilla.com> wrote:
> I'd honestly prefer to see this discussion drag on a little longer. The
> original email was on Friday, so given that most participants on this list
> aren't actively debating C++ coding style on the weekend, we've actually had
> less than one full working day of discussion on this issue so far.

And that's about as long as I think is worthwhile spending on the
discussion :) Feel free to continue discussing if you like, and if
there's a strong sentiment against the decision I made we can change
it.

> Let me suggest again (especially since I'm not sure my previous email
> reached this list yet) that rather than compromise on the "e" prefix, we
> compromise on all caps for values, which is just as readable without placing
> even more pressure on our 80-character line length restriction.

All caps actually makes long names *longer* because you now need to
add underscores to separate words, rather than using sentence case.
For example eSentenceCase is the same length as SENTENCE_CASE, but if
you tack on another word the "e" prefix is shorter. Presumably this
line-wrapping issue is more pronounced with long names than short
names, so this is actually an argument against all caps.

kats

[Jeff Gilbert]

On Mon, Apr 11, 2016 at 4:00 PM, Bobby Holley <bobby...@gmail.com> wrote:
> On Mon, Apr 11, 2016 at 2:12 PM, Jeff Gilbert <jgil...@mozilla.com> wrote:
>> I propose that if you're in a part of the codebase where you can't
>> tell if it's an enum or a function pointer (regardless of the fact
>> that the compiler can't confuse these), you should do more looking
>> around before working on it, much less reviewing changes to the code.
>> If you can't tell if it's an enum, you're not going to be able to
>> effectively make changes, so buckle up and do some looking around.
>
>
> I strongly disagree with this statement and the sentiment behind it. The
> ability for certain people to be able to quickly review changes in code that
> they don't have paged into working memory is vital to the survival of Gecko.

Well I disagree in the long term. If we are going to survive, we
*must* minimize such requirements. Skimping on code review should be
low on our list of options, given its propensity to create bugs and
accrue technical debt in the name of moving fast in the short term.

It's a tough job. I definitely understand. But good review is a
necessity. We have a number of problems that are "harsh realities",
but I hope we're trying to fix what we can, not optimizing assuming
failure.

> Mozilla is very bad at distributing review loads. There are literally a
> handful of people holding the ship together (and smaug is one of them). It
> is not a fun job, and the bus-factor is frighteningly low. This means that
> we should absolutely give more weight to the preferences of top reviewers
> because:
> (a) We owe it to them, and
> (b) We want to make it easier for other people to join their ranks.

Why do we have to be bad at distributing review loads? Is it a tooling
problem? And on-boarding problem? Why can't we reduce our need and/or
spread the load?

It's not pleasant to try to discuss these things only to be told that
I don't review enough code to matter. A bunch of us review code, and
yes, some more than others. I don't know where to find actual data,
but there is a panel helping with MozReview feedback whose members
were selected from 20 people chosen by virtue of being "top reviewers
by volume", a group which to which smaug, I, and at least 18 others
belong.

There are people (including smaug) who review more code than me. Their
opinions *do* have more weight than mine. However, I hope I my
arguments are still heard and granted what weight they are entitled
to, particularly since many people I talk with in person are strongly
opposed to changes here and elsewhere, but reluctant to join these
often-frustrating discussions.

It's always better when we have discussions based on data rather than
opinions, but when we're down to opinions, it's really hard to judge
support via email. There's no good "+1" option without spamming, and
many people just don't want to deal with the stress that merely
following these discussions can incur.

>> To go completely off the rails, I no longer even believe that all of
>> Gecko should have a single style.
>
>
> Yes, that is off the rails in this context.
>
>>
>> I think the whole attempt is
>> increasingly a distraction vs the alternative, and I say this as
>> someone who writes and reviews under at least three differently-styled
>> code areas. The JS engine will, as ever, remain distinct
>
>
> Jason agreed to converge SM style with Gecko.

The SM engineers I talked to are dismissive of this. I will solicit responses.

[Bobby Holley]

It's a hard problem we should do our best to address, but probably not in
this thread.

>
> It's not pleasant to try to discuss these things only to be told that
> I don't review enough code to matter. A bunch of us review code, and
> yes, some more than others. I don't know where to find actual data,
> but there is a panel helping with MozReview feedback whose members
> were selected from 20 people chosen by virtue of being "top reviewers
> by volume", a group which to which smaug, I, and at least 18 others
> belong.
>

To be very clear, I'm not suggesting you don't review a lot of code, or
that your vote doesn't matter. I am not saying anything about you
specifically. Rather, I am challenging your claims that:
(a) We shouldn't optimize for reviewers over developers, and
(b) Somebody reviewing code they don't have paged-in isn't doing a good job.

> There are people (including smaug) who review more code than me.

IMO, the thing that makes people like smaug and bz different from other
reviewers is not just the number of reviews they do, but the _breadth_ of
code they're expected to review. It's impossible to keep all that code
paged in, which is why consistency is so helpful.

> Their
> opinions *do* have more weight than mine. However, I hope I my
> arguments are still heard and granted what weight they are entitled
> to, particularly since many people I talk with in person are strongly
> opposed to changes here and elsewhere, but reluctant to join these
> often-frustrating discussions.
>
> It's always better when we have discussions based on data rather than
> opinions, but when we're down to opinions, it's really hard to judge
> support via email. There's no good "+1" option without spamming, and
> many people just don't want to deal with the stress that merely
> following these discussions can incur.
>

I totally agree. In my perfect world, the workflow would look like this:
(1) Somebody proposes a change to the style guide.
(2) Discussion ensues. If there's a clear consensus, end the algorithm.
(3) There's not a clear consensus. Somebody writes up a fair summary of the
pros and cons raised in the thread, ideally along with automated analysis
of the current state of the tree (i.e. X% of the enums are currently this
way, Y% that way). They post it to the thread, and ask if any rationale is
missing.
(4) Once the summary is deemed fair, we post it in a new thread, along with
a link to an external polling system. People are invited to vote, and may
optionally add a few characters of text to indicate which piece of
rationale was most instrumental in their decision.
(5) We analyze the results. If the results are pretty clear, end the
algorithm.
(6) If the poll results are still unclear, we hand the results to some
arbiter to decide (jst seems like a good choice). This person decides based
on poll results and pre-existing rationale (i.e. "No New Rationale").

Does that seem reasonable? None of it is hard, it just needs somebody to
shepherd it through.

[Jason Orendorff]

On Mon, Apr 11, 2016 at 7:58 PM, Jeff Gilbert <jgil...@mozilla.com> wrote:

> On Mon, Apr 11, 2016 at 4:00 PM, Bobby Holley <bobby...@gmail.com>
> wrote:
> > On Mon, Apr 11, 2016 at 2:12 PM, Jeff Gilbert <jgil...@mozilla.com>
> wrote:
> >> I think the whole attempt is
> >> increasingly a distraction vs the alternative, and I say this as
> >> someone who writes and reviews under at least three differently-styled
> >> code areas. The JS engine will, as ever, remain distinct
> >
> > Jason agreed to converge SM style with Gecko.
>
> The SM engineers I talked to are dismissive of this. I will solicit
> responses.
>

For the record:

SM is converging to Gecko style, at the rate that people contribute patches
to re-style our existing code. I.e., very slowly.

The one exception is indentation level: enough SM devs prefer 4-space
indents that we aren't changing that without further (internal) discussion.

-j