Re: Coding style for C++ enums

[Jason Orendorff]

On Mon, Apr 11, 2016 at 7:58 PM, Jeff Gilbert  wrote:

> On Mon, Apr 11, 2016 at 4:00 PM, Bobby Holley 
> wrote:
> > On Mon, Apr 11, 2016 at 2:12 PM, Jeff Gilbert 
> 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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[Bobby Holley]

On Mon, Apr 11, 2016 at 5:58 PM, Jeff Gilbert  wrote:

> On Mon, Apr 11, 2016 at 4:00 PM, Bobby Holley 
> wrote:
> > On Mon, Apr 11, 2016 at 2:12 PM, Jeff Gilbert 
> 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 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 

Re: Coding style for C++ enums

[Jeff Gilbert]

On Mon, Apr 11, 2016 at 4:00 PM, Bobby Holley  wrote:
> On Mon, Apr 11, 2016 at 2:12 PM, Jeff Gilbert  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.

>> , 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 

Re: Coding style for C++ enums

[Kartikaya Gupta]

On Mon, Apr 11, 2016 at 1:46 PM, Seth Fowler  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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[Bobby Holley]

On Mon, Apr 11, 2016 at 2:12 PM, 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. 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 
> 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  wrote:
> >> On 04/10/2016 05:50 PM, Aryeh Gregor wrote:
> >>>
> >>> On Fri, Apr 8, 2016 at 8:35 PM, smaug  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 

Re: Coding style for C++ enums

[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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[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  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  wrote:
> > On 04/10/2016 05:50 PM, Aryeh Gregor wrote:
> >>
> >> On Fri, Apr 8, 2016 at 8:35 PM, smaug  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.
> >>
> >
> > 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.
> >
> >
> >
> >
> >
> > ___
> > dev-platform mailing list
> > dev-platform@lists.mozilla.org
> > https://lists.mozilla.org/listinfo/dev-platform
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[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  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  wrote:
>> On 04/10/2016 05:50 PM, Aryeh Gregor wrote:
>>>
>>> On Fri, Apr 8, 2016 at 8:35 PM, smaug  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.
>>>
>>
>> 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.
>>
>>
>>
>>
>>
>> ___
>> dev-platform mailing list
>> dev-platform@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
> ___
> dev-platform mailing list
> 

Re: Coding style for C++ enums

[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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[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



—
- Milan




On Apr 11, 2016, at 11:00 , Kartikaya Gupta  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  wrote:

On 04/10/2016 05:50 PM, Aryeh Gregor wrote:


On Fri, Apr 8, 2016 at 8:35 PM, smaug  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.



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.





___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform




___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[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  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  wrote:
>> On 04/10/2016 05:50 PM, Aryeh Gregor wrote:
>>> 
>>> On Fri, Apr 8, 2016 at 8:35 PM, smaug  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.
>>> 
>> 
>> 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.
>> 
>> 
>> 
>> 
>> 
>> ___
>> dev-platform mailing list
>> dev-platform@lists.mozilla.org
>> https://lists.mozilla.org/listinfo/dev-platform
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[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


On Sun, Apr 10, 2016 at 11:43 AM, smaug  wrote:
> On 04/10/2016 05:50 PM, Aryeh Gregor wrote:
>>
>> On Fri, Apr 8, 2016 at 8:35 PM, smaug  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.
>>
>
> 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.
>
>
>
>
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[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  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  wrote:
>> 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
>> 
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[Aryeh Gregor]

On Fri, Apr 8, 2016 at 8:35 PM, smaug  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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[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.

On Fri, Apr 8, 2016 at 3:03 PM, Mats Palmgren  wrote:
> 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
>
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[Jonas Sicking]

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

Would you mind saying why?

/ Jonas
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[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

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[Jeff Gilbert]

Strong preference against eFoo, here. :)

Just use enum classes.

On Fri, Apr 8, 2016 at 10:35 AM, smaug  wrote:
> On 04/08/2016 07:38 PM, Nick Fitzgerald wrote:
>>
>> On Fri, Apr 8, 2016 at 9:29 AM, Birunthan Mohanathas <
>> birunt...@mohanathas.com> wrote:
>>
>>> On 8 April 2016 at 18:10, Kartikaya Gupta  wrote:

 Others?
>>>
>>>
>>> enum class OptionD {
>>>SentenceCaseValues,
>>>ThisWontBeConfusedWithOtherThings
>>>// ... because you need to use
>>> OptionD::ThisWontBeConfusedWithOtherThings
>>> };
>>>
>>
>> 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.
>>
>
> 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
>
> ___
> dev-platform mailing list
> dev-platform@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[smaug]

On 04/08/2016 07:38 PM, Nick Fitzgerald wrote:

On Fri, Apr 8, 2016 at 9:29 AM, Birunthan Mohanathas <
birunt...@mohanathas.com> wrote:


On 8 April 2016 at 18:10, Kartikaya Gupta  wrote:

Others?


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



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.



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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[Nick Fitzgerald]

On Fri, Apr 8, 2016 at 9:29 AM, Birunthan Mohanathas <
birunt...@mohanathas.com> wrote:

> On 8 April 2016 at 18:10, Kartikaya Gupta  wrote:
> > Others?
>
> enum class OptionD {
>   SentenceCaseValues,
>   ThisWontBeConfusedWithOtherThings
>   // ... because you need to use OptionD::ThisWontBeConfusedWithOtherThings
> };
>

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.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[smaug]

On 04/08/2016 06:10 PM, 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.

enum OptionA {
   ALL_CAPS_VALUES,
   THIS_IS_LIKE_SHOUTING
};

enum OptionB {
   SentenceCaseValues,
   ThisMightBeConfusedWithOtherThings
};

enum OptionC {
   eSentenceCaseValues,
   eThisSeemsPopular
};

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



___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: Coding style for C++ enums

[Birunthan Mohanathas]

On 8 April 2016 at 18:10, Kartikaya Gupta  wrote:
> Others?

enum class OptionD {
  SentenceCaseValues,
  ThisWontBeConfusedWithOtherThings
  // ... because you need to use OptionD::ThisWontBeConfusedWithOtherThings
};
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

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
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform