1577413 - Define process to evolve and implement component-specific coding style

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, task, P2)

Description

[Simon Giesecke [:sg] [he/him]]

A lightweight process should be described as part of the component-specific coding style document to evolve it, in two directions:

• Increase degree of conformance of the actual code with the documented coding style
• Amend the documented coding style to better document the coding style as intended by shared understanding of the main contributors

The first point may include actions as part of other bugs or specific bugs to resolve non-conformance issues.

The second point may include actions related to the reviewing process.

Comment 1

[Simon Giesecke [:sg] [he/him]]

A third point that combines the two mentioned above would be how to change existing rules that have been both documented and (mostly) implemented. This should be decoupled from the review process. One reason for that is that performing such discussions within a specific review would possibly block the review just for this reason, which is not desirable.

Comment 2

[Simon Giesecke [:sg] [he/him]]

@perry, you asked the following question (I edited it a bit to resolve some ambiguity):

"When writing [code] with an updated style, is the author [of that code] responsible for updating all local (say, within the current file) violations? Extra work that's not necessarily relevant to the bug at hand, but makes subsequent reading more consistent."

I would say that the author is not obliged but encouraged to do so, time permitting. Whether that's the case depends on the size of the file, the number of violations and the complexity and risk of performing the changes, which can be left to the judgment of the code author IMO.

I will include this in the description of the evolution process, maybe in some FAQ section.

You also made some suggestions for the content:

• prefer const/constexpr over macros: This is already mentioned in the Google C++ coding style at https://google.github.io/styleguide/cppguide.html#Preprocessor_Macros
• strongly typed enums (i.e. scoped enums) over old enums: I will create a bug to include this in our style.
• no 200 line functions: The Google C++ coding style encourages "short" functions at https://google.github.io/styleguide/cppguide.html#Write_Short_Functions, but does explicitly not mention a particular length. I would make this a bit stricter. A hard limit cannot be set, probably. But a soft limit of much less than 200 lines (maybe 40 lines?) would be good, encouraging the author/reviewer to seriously reconsider if something should be done about the length of the function in that case. I will create a bug to include a rule related to this in our style.
• comments where needed: This is already mentioned in the Google C++ coding style at https://google.github.io/styleguide/cppguide.html#Comments (Comments are absolutely vital to keeping our code readable. The following rules describe what you should comment and where. But remember: while comments are very important, the best code is self-documenting. Giving sensible names to types and variables is much better than using obscure names that you must then explain through comments.)
• well-defined order for #includes: The Google C++ coding style defines an order for includes at https://google.github.io/styleguide/cppguide.html#Comments. As you mention, clang-format can be configured to automatically order and group includes (at least where they are not conditional). However, applying this ubiquitously and/or automatically requires self-contained headers, which we reportedly do not have everywhere (I am not sure where such exceptions are, and if they are relevant to our modules).
• class structure public/protected/private: This is already mentioned in the Google C++ coding style at https://google.github.io/styleguide/cppguide.html#Declaration_Order (second paragraph)

It might be helpful to explicitly reinforce points particular important to us (or which are often neglected) that are already covered by the Google C++ Coding Style (with a reference to that style).

@perry, please briefly acknowledge here that I address your feedback as intended, or comment on what I got wrong. Thanks!

Comment 3

[Perry Jiang [:perry] [no longer employee, use ni?]]

Feedback addressed!

(In reply to Simon Giesecke [:sg] [he/him] from comment #2)

I would say that the author is not obliged but encouraged to do so, time permitting. Whether that's the case depends on the size of the file, the number of violations and the complexity and risk of performing the changes, which can be left to the judgment of the code author IMO.

Yeah, I agree. There isn't a clear definition for what a "trivial cleanup" is. The example I was thinking of is if an author introduces duplication that exists that didn't exist before the change, he/she should be responsible for factoring out the duplication into shared code (i.e. we may want to enforce the standard that no change should decrease the overall quality).

Comment 4

[Simon Giesecke [:sg] [he/him]]

@jstutte, you made the following comments last week:

• "Have the coding style open at reviews, amend it as needed. How to agree in team?" The first sentence should be included in the review process description (I am not sure if you took on to write that down?) I am not sure what the question in the second sentence refers to. Agree on what?
• "Who should do 'coding style update' review? All team? Vote?" That's a good question, and it should be addressed in the coding style evolution process description I am going to write to resolve this bug. I think changes to the coding style should be reviewed on the one hand by an owner of the coding style document (which I would volunteer to be for now), on the other hand by someone with good familiarity with large parts of the local code base, and if either coincides with the author of the coding style change, by two other team members in total. Requiring the complete team to review or vote would pose too many obstacles IMO. The coding style can be changed, so any concerns can be raised at any time after an amendment has been included in the style document originally.

@jstutte please respond to my question in the first bullet point, and acknowledge or comment on my suggestion in the second bullet point.

Comment 5

[Simon Giesecke [:sg] [he/him]]

(In reply to Perry Jiang [:perry] from comment #3)

Feedback addressed!

(In reply to Simon Giesecke [:sg] [he/him] from comment #2)

I would say that the author is not obliged but encouraged to do so, time permitting. Whether that's the case depends on the size of the file, the number of violations and the complexity and risk of performing the changes, which can be left to the judgment of the code author IMO.

Yeah, I agree. There isn't a clear definition for what a "trivial cleanup" is. The example I was thinking of is if an author introduces duplication that exists that didn't exist before the change, he/she should be responsible for factoring out the duplication into shared code (i.e. we may want to enforce the standard that no change should decrease the overall quality).

Maybe I got your comment a bit wrong. I was rather thinking of something that is newly added to the style (e.g. use scoped enums). If someone adds a scoped enum to some class for example, should they change all existing enums in that class to scoped enums, too? While is straightforward and not risky, whether this can be done on-the-go still depends on the number of uses of the enum values.

Duplicated code is a rather special case, which is somewhat at the frontiers of coding style. Personally, I would rather aggressively resolve code duplication whenever possible. But it may not even be obvious when writing code that it introduces a duplication (except when it is created through copy-and-paste).

Comment 6

[Perry Jiang [:perry] [no longer employee, use ni?]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #5)

Maybe I got your comment a bit wrong. I was rather thinking of something that is newly added to the style (e.g. use scoped enums). If someone adds a scoped enum to some class for example, should they change all existing enums in that class to scoped enums, too? While is straightforward and not risky, whether this can be done on-the-go still depends on the number of uses of the enum values.

I would say it's not their responsibility because not doing so doesn't decrease overall quality. (My original comment could've definitely been clearer.)

Comment 7

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1577413 - Add initial description of code style evolution process. r=jstutte

Comment 8

[Jens Stutte [:jstutte]]

I did not write down anything, yet. I have the cards here with me, though, and want to do this asap.

Comment 9

[Jens Stutte [:jstutte]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #4)

@jstutte, you made the following comments last week:

• "Have the coding style open at reviews, amend it as needed. How to agree in team?" The first sentence should be included in the review process description (I am not sure if you took on to write that down?) I am not sure what the question in the second sentence refers to. Agree on what?

The question was, what should the process look like to agree on amendments to the coding style in order to ensure acceptance by the team. You already have written an answer to this with the bugzilla process and double r+, which should ensure an acceptable level of sharing without blocking everybody.

• "Who should do 'coding style update' review? All team? Vote?" That's a good question, and it should be addressed in the coding style evolution process description I am going to write to resolve this bug. I think changes to the coding style should be reviewed on the one hand by an owner of the coding style document (which I would volunteer to be for now), on the other hand by someone with good familiarity with large parts of the local code base, and if either coincides with the author of the coding style change, by two other team members in total. Requiring the complete team to review or vote would pose too many obstacles IMO. The coding style can be changed, so any concerns can be raised at any time after an amendment has been included in the style document originally.

You already wrote this process, and it seems reasonable to me. I would give it a try like this and adjust if we encounter problems.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0c1317f4fe0
Add initial description of code style evolution process. r=jstutte

Comment 11

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/b0c1317f4fe0