On Mon, Apr 11, 2016 at 2:12 PM, Jeff Gilbert <jgilb...@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:
> >> On 04/10/2016 05:50 PM, Aryeh Gregor wrote:
> >>>
> >>> 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.
> >>>
> >>
> >> 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
>
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
