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