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