Bug 31618 – "using" statement coding style guidelines need clarification

• Component: New Bugs
• Status: RESOLVED FIXED

[Chris Jerdonek]

I recently learned from Darin Adler on the webkit-dev list that there are a few more "unwritten rules" regarding "using" statements in header files.  Namely--

(1) "using namespace" directives are unacceptable only if in the global scope (the guidelines say they are always unacceptable).
(2) The rules should probably apply not just to "using namespace" directives but also to "using" declarations.
(3) Statements of the form "using WTF::..." are exceptions.

Also, the coding style page does not validate.

A patch is forthcoming.

[Chris Jerdonek]

Created attachment 43413 [details]
Proposed patch

(1) This patch corrects and clarifies the style guidelines re: "using" statements, per the report.

(2) It also clarifies a part of the web page about code cleanup:

http://webkit.org/projects/cleanup/index.html

The web site currently gives the impression that the WebKit project wants patches whose sole purpose is to clean up the code, but Alexey and Mark clarified otherwise--

https://bugs.webkit.org/show_bug.cgi?id=31526

(3) The style page now validates.

(4) The patch clarifies on the web site that JavaScriptGlue does not conform well to the guidelines (per Darin's e-mail on webkit-dev).

(I am still fuzzy on whether JSG does not conform because it is exempt from the guidelines -- so that it will never conform, or because less work has gone into it.  If the former, the site should probably state that somewhere so people don't invest unnecessary work in trying to clean it up.)

[Darin Adler]

(In reply to comment #1)
> (I am still fuzzy on whether JSG does not conform because it is exempt from the
> guidelines -- so that it will never conform, or because less work has gone into
> it.  If the former, the site should probably state that somewhere so people
> don't invest unnecessary work in trying to clean it up.)

JavaScriptGlue is a legacy component that should not receive any new development. It's needed for compatibility with some older Mac OS X software, but will eventually be retired.

Changes to it should be kept to a minimum. Style improvements would be counterproductive.

[Darin Adler]

Comment on attachment 43413 [details]
Proposed patch

> +<li>It is acceptable, however, to use "using" declarations at the end of 
> +header files to include into the global scope one or more names in 
> +the WTF namespace.

I would specifically call this out as an exception to the rule just for the WTF sub-library.

> +<pre class="code">
> +// Vector.h
> +
> +namespace WTF {
> +
> +} // namespace WTF
> +
> +using namespace WTF;
> +
> +
> +// Quantifier.h
> +
> +namespace JSC {
> +
> +} // namespace JSC
> +
> +using JSC::Quantifier;
> +</pre>

This repeats the Right example inside the Wrong!

>  <p>In order for your patch to be landed, it's necessary that it comply to the <a href="/coding/coding-style.html">code style guidelines</a>.
> -There are some older parts of the codebase that do not always follow these guidelines. If you come across code like this,
> +There are some older parts of the codebase that do not always follow 
> +these guidelines. JavaScriptGlue is one example. If you come across code like this,
>  it's generally best to clean it up to comply with the new guidelines.</p>

I don't think this is good. JavaScriptGlue is different. It's end-of-lifed code, not just an older part of the codebase.

>  <dt>Follow the Coding Style Guidelines</dt>
>  <dd>We welcome patches that clean up code to follow our coding style guidelines.
> +However, cleanup should ordinarily take place only when you are already 
> +touching a certain area of code.</dd>

I know this is what we discussed, but I think it's confusing rule. Can we do better?

[Chris Jerdonek]

(In reply to comment #2)
> JavaScriptGlue is a legacy component that should not receive any new
> development. It's needed for compatibility with some older Mac OS X software,
> but will eventually be retired.
> 
> Changes to it should be kept to a minimum.

Thanks.  This is new information to me that I didn't pick up from reading the web site.  I think this is good information for someone new to the project that should appear on the web site somewhere.  Perhaps here--

http://trac.webkit.org/wiki/HighLevelOverview

Can you think of a better place?

[Chris Jerdonek]

(In reply to comment #3)
> (From update of attachment 43413 [details])
> > +<li>It is acceptable, however, to use "using" declarations at the end of 
> > +header files to include into the global scope one or more names in 
> > +the WTF namespace.
> 
> I would specifically call this out as an exception to the rule just for the WTF
> sub-library.

Sounds good.

> This repeats the Right example inside the Wrong!

The Wrong is actually slightly different -- including not just one name in the WTF namespace but the entire WTF namespace (or is that okay?):

+<h4 class="right">Right:</h4>
+
+} // namespace WTF
+
+using WTF::Vector;

+<h4 class="wrong">Wrong:</h4>
+
+} // namespace WTF
+
+using namespace WTF;

> >  <dt>Follow the Coding Style Guidelines</dt>
> >  <dd>We welcome patches that clean up code to follow our coding style guidelines.
> > +However, cleanup should ordinarily take place only when you are already 
> > +touching a certain area of code.</dd>
> 
> I know this is what we discussed, but I think it's confusing rule. Can we do
> better?

I guess I need your help here.  I wrote this partly to gain clarification: is it or is it not okay to submit patches that address only style?  The web site seems to say (even encourage) yes, but Alexey and Mark's response to my request above seems to say no.  I think it would be helpful for the site to be clearer on this point.

(Personally, I was only planning to do this in cases where things can be cleaned up with a script either globally or per-project, etc.  For example--

https://bugs.webkit.org/show_bug.cgi?id=31167

This seems more efficient than having everyone do a little bit at a time.  Also, in some cases it helps to have the style patches separated from the functional patches anyways.)

[Darin Adler]

(In reply to comment #4)
> Can you think of a better place?

No, but I'm not really familiar with our documentation structure, believe it or not!

[Darin Adler]

(In reply to comment #5)
> The Wrong is actually slightly different -- including not just one name in the
> WTF namespace but the entire WTF namespace (or is that okay?):
> 
> +<h4 class="wrong">Wrong:</h4>
> +
> +} // namespace WTF
> +
> +using namespace WTF;

I think we need two different Wrong for the two different mistakes. Instead it looked like a since Wrong case that demonstrated two errors. A reader might miss one or the other other.

> > >  <dt>Follow the Coding Style Guidelines</dt>
> > >  <dd>We welcome patches that clean up code to follow our coding style guidelines.
> > > +However, cleanup should ordinarily take place only when you are already 
> > > +touching a certain area of code.</dd>
> > 
> > I know this is what we discussed, but I think it's confusing rule. Can we do
> > better?
> 
> I guess I need your help here.  I wrote this partly to gain clarification: is
> it or is it not okay to submit patches that address only style?  The web site
> seems to say (even encourage) yes, but Alexey and Mark's response to my request
> above seems to say no.  I think it would be helpful for the site to be clearer
> on this point.
> 
> (Personally, I was only planning to do this in cases where things can be
> cleaned up with a script either globally or per-project, etc.  For example--
> 
> https://bugs.webkit.org/show_bug.cgi?id=31167
> 
> This seems more efficient than having everyone do a little bit at a time. 
> Also, in some cases it helps to have the style patches separated from the
> functional patches anyways.)

Obviously, I can't make this decision on my own. Alexey and Mark are both influential, long-term contributors and have a lot to say on how the project is run. And so do I.

I thought of a few pros:

    1) Fixing formatting in earlier patches keeps formatting changes to a minimum in future patches that include substantive changes.

    2) It's good to have as much code as possible exemplify the style for new contributors.

    3) Good to eliminate distracting style differences while reading the code.

And cons:

    A) Changes en masse may make things worse and less readable without the people applying the changes noticing. Doing style changes in smaller pieces helps us notice cases where our style guidelines create poor results.

    B) Code style patches harm features like Subversion's "annotate", giving the wrong information about who worked on code most recently.

    C) Applying code style from a document to the entire tree makes it easy for a small mistake in a document turn into a large problem in many source files.

    D) Encourages people to think of the guidelines as a mechanical thing for programs and scripts to deal with like the rules of a programming language rather than a shared set of rules made by people for the benefit of other people.

[Chris Jerdonek]

(In reply to comment #7)
> I think we need two different Wrong for the two different mistakes.

Sounds good.

> > I guess I need your help here.  I wrote this partly to gain clarification: is
> > it or is it not okay to submit patches that address only style?
> > ...
> Obviously, I can't make this decision on my own. Alexey and Mark are both
> influential, long-term contributors and have a lot to say on how the project is
> run. And so do I.
> 
> I thought of a few pros:
> 
> ...
> 
> And cons:
> 
> ...

This is an excellent list of pros and cons.  Given that there is no clear answer (and that I don't think it's worth the group's time to try to come up with one right now), I might just add a note warning the would-be contributor of style-only patches.

By the way, two more cons:

E) Lessens the incentive for people to become personally responsible for conformance to the guidelines.

F) Reduces the number of opportunities for people to become more familiar with the guidelines in a hands-on way.

Finally, providing a script that other people can use on a per-folder basis (for example) would mitigate cons A, B, and C.  This seems similar to the approach taken with the do-webcore-rename script in the WebKitTools/Scripts folder.

[Chris Jerdonek]

Created attachment 43694 [details]
Revised patch

I have incorporated Darin's comments.

A couple remarks:

1) I'm not sure if I interpreted the following suggestion correctly, after all:

> I think we need two different Wrong for the two different mistakes. Instead it
> looked like a since Wrong case that demonstrated two errors. A reader might
> miss one or the other other.

I took this to mean to add a second "Wrong" header for the second mistake instead of having a single "Wrong" header for both mistakes.  I'm not sure if this is what was intended, though, since the rest of the Style Guidelines page always uses a single "Wrong" header to illustrate mistakes -- even a group of them.

2) Regarding Darin's information that JavaScriptGlue is a legacy component, I added this information to the WebKit Wiki ( http://trac.webkit.org/wiki/HighLevelOverview ) instead of including it in this patch.

Thanks.

[WebKit Commit Bot]

Comment on attachment 43694 [details]
Revised patch

Clearing flags on attachment: 43694

Committed r51587: <http://trac.webkit.org/changeset/51587>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.