Consensus on when to use "using" directives

[Peter Kasting]

We don't seem to have a definitive policy on when "using" directives can be used, and I'd like to try and reach consensus.

Here I am referring to things like "using foo::Bar;" in a .cc file.  Note that both "using namespace foo;" and the use of "using" directives in header files are already banned, so I'm not talking about those.

Here are some possibilities:

  * Never use "using".  Pros: Easiest to consistently enforce, eliminates possibilities of name conflicts.  Cons: Could lead to verbose code and/or hinder readability.

  * Use "using" when adding a namespace to objects that didn't use to have one, in order to avoid having to add qualifiers on all uses everywhere.  Pros: Minimizes change deltas for cleanups that add namespaces.  Cons: No guarantees that the directives will eventually be removed, so the codebase can be left in a somewhat inconsistent state; also the cons of the next item.

  * Use "using" when there are "many" (reviewers' discretion) uses of a qualified object in the file.  Pros: Maximal "bang for the buck" in terms of code conciseness.  Cons: Impossible to enforce consistently, may confuse readers when objects have qualifiers in some files and not others ("is this the same class?"), inertia will keep files either having these directives or not even if the number of references in the file changes dramatically.

  * Use "using" in all cases for particular namespaces.  I have heard that this policy is in place for Chromium code that uses the "webkit::" WebKit API namespace objects, but I don't know what prompted it if so.  Pros: ?.  Cons: ?.  Please speak up (Darin?) about this one.

This question is prompted by John Abd-El-Malek's work on the Content API, wherein he's recently begun changing to using "using" directives everywhere to match the perceived style of the WebKit API (see e.g. http://codereview.chromium.org/8983012 ).  Before we get too far into this I wanted to make sure it's actually what we want.

My personal preference is to never use "using" at all, and I've tried over the years to keep it out of all the files I touch.  It's rare for it to save a significant number of lines of code, and it's much clearer to me as a reader when a particular class always appears with the same qualifiers.  (Whenever I run into code that e.g. uses the STL's "find()" function without "std::" in front I always wonder what particular find() is being called.)  I can see "using" being beneficial in rare cases where code external to a namespace makes heavy use of an object inside the namespace, and the qualified name is very long; but the consistency and ease of just enforcing "don't use this" has always seemed superior to me.  However, that's not official team style, so I'd like to see if we have a good feel for whether it, or some other set of rules, should be.

PK

[George Yakovlev]

+1 to *never* (or very rarely) use "using"

Especially with third_party libraries that we cannot control there are many possibilities for accidental name conflict.

Additional plus of being verbose is the easier reading of the code.

Thanks,

  George.

PK

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Chris Palmer]

What about e.g.

typedef std::vector<breakfast::Crumpet> Crumpets;

I like it better than "using", because it's more effective at making
the code readable and less effective at poisoning the namespace. (For
example, "using" doesn't do enough to shorten
vector<Crumpet>::const_iterator, but Crumpets::const_iterator is
pretty reasonable.)

[Peter Kasting]

On Thu, Jan 5, 2012 at 2:10 PM, Chris Palmer <pal...@google.com> wrote:

What about e.g.

   typedef std::vector<breakfast::Crumpet> Crumpets;

I use this pattern all the time and encourage others to use it, especially when the relevant type is exposed in part of a class' public API.  It not only makes it less verbose to use iterators with the object, it also makes it easier to shield external users from the implementation of the object, which comes in handy if down the road you need to change from e.g. a vector to a list.

PK

[Ben Goodger (Google)]

+1 to not using using.

I find it slightly more useful to know where things come from.

-Ben

On Thu, Jan 5, 2012 at 1:59 PM, Peter Kasting <pkas...@google.com> wrote:

PK

--

[Scott Hess]

On Thu, Jan 5, 2012 at 1:59 PM, Peter Kasting <pkas...@google.com> wrote:

> My personal preference is to never use "using" at all, and I've tried over
> the years to keep it out of all the files I touch.  It's rare for it to save
> a significant number of lines of code, and it's much clearer to me as a
> reader when a particular class always appears with the same qualifiers.

Never using "using" also provides a gentle discouragement to going too
apeshit with namespaces.

-scott

[Peter Kasting]

I forgot to mention.  Effective C++ does define a few cases where "using" is a good way to accomplish something tricky, e.g. in items 25, 33, and 43.  My suspicion is that these cases are narrow enough that we don't really need to worry about them when setting style rules.

PK

[Ilya Sherman]

On Thu, Jan 5, 2012 at 1:59 PM, Peter Kasting <pkas...@google.com> wrote:

We don't seem to have a definitive policy on when "using" directives can be used, and I'd like to try and reach consensus.

Here I am referring to things like "using foo::Bar;" in a .cc file.  Note that both "using namespace foo;" and the use of "using" directives in header files are already banned, so I'm not talking about those.

Here are some possibilities:

  * Never use "using".  Pros: Easiest to consistently enforce, eliminates possibilities of name conflicts.  Cons: Could lead to verbose code and/or hinder readability.

  * Use "using" when adding a namespace to objects that didn't use to have one, in order to avoid having to add qualifiers on all uses everywhere.  Pros: Minimizes change deltas for cleanups that add namespaces.  Cons: No guarantees that the directives will eventually be removed, so the codebase can be left in a somewhat inconsistent state; also the cons of the next item.

  * Use "using" when there are "many" (reviewers' discretion) uses of a qualified object in the file.  Pros: Maximal "bang for the buck" in terms of code conciseness.  Cons: Impossible to enforce consistently, may confuse readers when objects have qualifiers in some files and not others ("is this the same class?"), inertia will keep files either having these directives or not even if the number of references in the file changes dramatically.

I strongly prefer this option, i.e. use of "using" directives when they are deemed helpful.  For example, the Autofill code uses webkit::forms::FormData objects, throughout.  "using" directives save literally hundreds of line wraps in this code, which IMO hurt readability more than a lack of fully qualified names.

 

  * Use "using" in all cases for particular namespaces.  I have heard that this policy is in place for Chromium code that uses the "webkit::" WebKit API namespace objects, but I don't know what prompted it if so.  Pros: ?.  Cons: ?.  Please speak up (Darin?) about this one.

This question is prompted by John Abd-El-Malek's work on the Content API, wherein he's recently begun changing to using "using" directives everywhere to match the perceived style of the WebKit API (see e.g. http://codereview.chromium.org/8983012 ).  Before we get too far into this I wanted to make sure it's actually what we want.

My personal preference is to never use "using" at all, and I've tried over the years to keep it out of all the files I touch.  It's rare for it to save a significant number of lines of code, and it's much clearer to me as a reader when a particular class always appears with the same qualifiers.  (Whenever I run into code that e.g. uses the STL's "find()" function without "std::" in front I always wonder what particular find() is being called.)  I can see "using" being beneficial in rare cases where code external to a namespace makes heavy use of an object inside the namespace, and the qualified name is very long; but the consistency and ease of just enforcing "don't use this" has always seemed superior to me.  However, that's not official team style, so I'd like to see if we have a good feel for whether it, or some other set of rules, should be.

PK

--

[John Abd-El-Malek]

note: by "perceived", this is the convention I have heard from Darin. For the WebKit API, initially I thought it was a little strange to do this each time I used an interface. But after a while I liked it (reasons below). For the Content API, we're trying to be as similar to the WebKit API as possible, to not add yet another convention, which is why we were doing this.

For the record, I don't think we should do this for third party or for every function that gets put in a namespace for scoping. However I think for large modules with APIs, it makes things easier to type and read, especially with our 80 char limit. The public APIs should have a small surface, by definition, so it should be obvious to someone reading  the code where the class is from. If not, then even without a namespace they'd still have to use their IDE to get to the declaration.

On Thu, Jan 5, 2012 at 1:59 PM, Peter Kasting <pkas...@google.com> wrote:

--

[Victor Khimenko]

On Fri, Jan 6, 2012 at 2:21 AM, Ilya Sherman <ishe...@chromium.org> wrote:

I strongly prefer this option, i.e. use of "using" directives when they are deemed helpful.  For example, the Autofill code uses webkit::forms::FormData objects, throughout.  "using" directives save literally hundreds of line wraps in this code, which IMO hurt readability more than a lack of fully qualified names.

This can be solved in cleaner way with "using namespace" - but sadly these are already prohibited.

namespace wf {

  using namespace webkit::forms;

}

wf::FormData ...

[Ilya Sherman]

Conversely, I find this pattern to be incredibly frustrating when reading code.  I can't tell from glancing at the typename Crumpets whether it logically behaves like a list, like a set, or like a map.  I pretty much always end up hunting down the typedef to double-check, and am surprised about 1/3 of the time.  I think this would be less of an issue if we had a consistent naming scheme for such typedefs, e.g. "Crumpets" for a list-like container, "CrumpetSet" for a set-like container, "CrumpetMap" for a map-like container.

Also, as a minor quibble to Peter's point: I've often seen vectors typedef'd in this way, but then coupled with uses of Crumpets::reserve(n); -- so the code would break if you do change the type from a vector to a list.  If you want to shield clients of your public API, please consider creating a proper wrapper class ;)

[Ami Fischman]

I strongly prefer this option, i.e. use of "using" directives when they are deemed helpful.  For example, the Autofill code uses webkit::forms::FormData objects, throughout.  "using" directives save literally hundreds of line wraps in this code, which IMO hurt readability more than a lack of fully qualified names.

This can be solved in cleaner way with "using namespace" - but sadly these are already prohibited.

namespace wf {

  using namespace webkit::forms;

}

wf::FormData ...

But this form:

namespace wf = webkit::forms;

seems commonish: http://code.google.com/codesearch#search/&exact_package=chromium&q=^namespace.*=%20case%3Ayes%20file%3A%5C.cc&type=cs

-a

[Peter Kasting]

On Thu, Jan 5, 2012 at 2:33 PM, Ilya Sherman <ishe...@chromium.org> wrote:

Conversely, I find this pattern to be incredibly frustrating when reading code.  I can't tell from glancing at the typename Crumpets whether it logically behaves like a list, like a set, or like a map.  I pretty much always end up hunting down the typedef to double-check, and am surprised about 1/3 of the time.  I think this would be less of an issue if we had a consistent naming scheme for such typedefs, e.g. "Crumpets" for a list-like container, "CrumpetSet" for a set-like container, "CrumpetMap" for a map-like container.

I couldn't say for certain, but I think my normal tendency is to use "Crumpets" for a vector and/or list and "CrumpetMap" for a map.  Semantically, it kind of reads weird to say "Crumpets" in many cases where you're doing map-like operations.  I'd be happy to support this informal suggestion for naming :)

Also, as a minor quibble to Peter's point: I've often seen vectors typedef'd in this way, but then coupled with uses of Crumpets::reserve(n); -- so the code would break if you do change the type from a vector to a list.  If you want to shield clients of your public API, please consider creating a proper wrapper class ;)

Yeah, it depends how far you want to go.  I'm not usually worried about complete implementation hiding, just making callers "more agnostic", especially when it seems like usually it's code internal to the implementation that does things like reserve().

PK

[Dominic Mazzoni]

FYI, Google's C++ Style Guide (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml) has clear guidance on "using". The quick summary is that you may not use a using-directive to make all names from a namespace available, but you can use it in a .cc file to make individual names available.

// Forbidden -- This pollutes the namespace.
using namespace foo;

// OK in .cc files.
using ::foo::bar;

I like this compromise, because it means that you never have to search farther than the same source file to find the complete name of something, and anyone involved in refactoring who searches for the full name (webkit::npapi::PluginGroup) will be guaranteed to find the full name in any source file that uses it.

It's fine with me if using-directives are discouraged - but I think we should make a strong distinction between including a whole namespace and including a single name at a time. The former should be forbidden unless it's a project-wide convention that everyone agrees on, whereas the latter could be discouraged but allowed when it really helps readability and line wrapping.

- Dominic

[Peter Kasting]

On Thu, Jan 5, 2012 at 2:58 PM, Dominic Mazzoni <dmaz...@google.com> wrote:

FYI, Google's C++ Style Guide (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml) has clear guidance on "using". The quick summary is that you may not use a using-directive to make all names from a namespace available, but you can use it in a .cc file to make individual names available.

Yes; that's how we can have any code at all today that uses this.

I'm trying to get a more detailed position that goes beyond the base Google guide.

It's fine with me if using-directives are discouraged - but I think we should make a strong distinction between including a whole namespace and including a single name at a time.

We already do.  The former is already forbidden, as I noted in the original post (and Nico also commented on).

 

the latter could be discouraged but allowed when it really helps readability and line wrapping.

That's precisely what one of the possible outcomes I listed is.

PK

[Gregg Tavares (wrk)]

If we're going to be strict on no 'using' that includes gmock

If gmock merits an exception that seems to argue there are other reasons to have exceptions.

[Peter Kasting]

On Thu, Jan 5, 2012 at 3:08 PM, Gregg Tavares (wrk) <gm...@google.com> wrote:

If we're going to be strict on no 'using' that includes gmock

If gmock merits an exception that seems to argue there are other reasons to have exceptions.

I don't know a thing about gmock.  If it qualifies as third_party code, then I would be inclined to ignore it, as we don't try to enforce the Chromium style guide on non-Chromium code.

PK

[Gregg Tavares (wrk)]

I mean usage of gmock

  EXPECT_CALL(

      *gl_, GetShaderInfoLog(kServiceShaderId, strlen(kInfo) + 1, _, _))

      .WillOnce(DoAll(SetArgumentPointee<2>(strlen(kInfo)),

                      SetArrayArgument<3>(kInfo, kInfo + strlen(kInfo) + 1)));

Becomes

  EXPECT_CALL(

      *gl_, GetShaderInfoLog(kServiceShaderId, strlen(kInfo) + 1, _, _))

      .testing::WillOnce(testing::DoAll(
          testing::SetArgumentPointee<2>(strlen(kInfo)),

          testing::SetArrayArgument<3>(kInfo, kInfo + strlen(kInfo) + 1)));

[Albert Bodenhamer]

I'd support exceptions if the author of the code can make a strong case that using "using" makes the code significantly easier to read/maintain.  Such cases should be exceedingly rare.  

Usually, if the code is cleaner with a "using" than without it that's a strong argument that there's something wrong with the design.

On Thu, Jan 5, 2012 at 3:08 PM, Gregg Tavares (wrk) <gm...@google.com> wrote:

If we're going to be strict on no 'using' that includes gmock

If gmock merits an exception that seems to argue there are other reasons to have exceptions.

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
Albert Bodenhamer | Software Engineer | abodenha@chromium.org 

[John Abd-El-Malek]

On Thu, Jan 5, 2012 at 3:15 PM, Albert Bodenhamer <abod...@chromium.org> wrote:

I'd support exceptions if the author of the code can make a strong case that using "using" makes the code significantly easier to read/maintain.  Such cases should be exceedingly rare.  

Usually, if the code is cleaner with a "using" than without it that's a strong argument that there's something wrong with the design.

That's quite a strong statement, can you elaborate?

The combination of descriptive class/method names, 80 character limits and namespaces for major modules' API makes wrapping excessive.

[Gary Kacmarcik]

On Thu, Jan 5, 2012 at 3:01 PM, Peter Kasting <pkas...@google.com> wrote:

On Thu, Jan 5, 2012 at 2:58 PM, Dominic Mazzoni <dmaz...@google.com> wrote:

FYI, Google's C++ Style Guide (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml) has clear guidance on "using". [...]

Yes; that's how we can have any code at all today that uses this.

I interpreted this reference to the style guide as a counterpoint to your earlier comment:

We don't seem to have a definitive policy on when "using" directives can be used, and I'd like to try and reach consensus.

I've always thought that we did in fact have a well-defined policy WRT the "using" directive (ie: the one mentioned in style guide). And the fact that our code doesn't have places where we use "using" in .h files or for entire namespaces supports the position that that's the common belief.

The current "use them only in .cc files and only when they improve readability" policy seems perfectly fine to me.  I haven't seem places where it's being abused.

-Gary

[Albert J. Wong (王重傑)]

I'm not sure I see the harm in having individual "using statements" inside a .cc file or inside a function scope significantly hurts readability.  As mentioned previously in the thread, your search space is limited to the current file which is basically the same as if you had declared a static utility method.

On the otherhand, you trade call-site readability due to verbose line length.  Gmock is a strong example of where this trade off is ugly.  So is all the base::Bind(), base::Unretained() stuff.  But even in more "normal code", I'm not sure what is gained by forcing people in .cc files to type-out the namespaces in base::WeakPtr, base::MessageLoopProxy, file_util::ContainsPath, etc.  You don't have a viral namespace merge and in repetitive code the namespace becomes visual clutter.

IMO, we actually don't quite have enough "using statements."  :)

The status quo of banning them in headers, and then leaving them to author/reviewer discretion in the .cc files seems like a good balance.  I don't think the incremental consistency we'd gain by legislating a hard rule offsets the readability lost in situations that use lots of functions from another namespace.

-Albert

[Chris Palmer]

On Thu, Jan 5, 2012 at 3:23 PM, John Abd-El-Malek <j...@chromium.org> wrote:

> The combination of descriptive class/method names, 80 character limits and
> namespaces for major modules' API makes wrapping excessive.

Yes. And of those things, it's the 80-character limit I'd get rid of.
Our screens became wider than 80 columns around the same time our
compilers started distinguishing more than 6 characters in
identifiers, but only one practice changed with the times. :) Although
a certain width limit makes sense for natural language text (see e.g.
http://baymard.com/blog/line-length-readability), we read code
differently and line breaks hurt more than the occasional long line.
Probably the indentation helps guide us, but I don't know.

[Peter Kasting]

On Thu, Jan 5, 2012 at 3:34 PM, Chris Palmer <pal...@google.com> wrote:

On Thu, Jan 5, 2012 at 3:23 PM, John Abd-El-Malek <j...@chromium.org> wrote:

> The combination of descriptive class/method names, 80 character limits and
> namespaces for major modules' API makes wrapping excessive.

Yes. And of those things, it's the 80-character limit I'd get rid of.

The 80-column limit is not up for debate in this thread.

If I sound like a jerk saying that, it's because I absolutely do not want a free-for-all bikeshed debate about multiple different aspects of the style guide.  If you want this changed, I suggest starting a new thread.  (Actually I suggest not wasting your time, but whatever.)

PK

[Peter Kasting]

On Thu, Jan 5, 2012 at 3:23 PM, John Abd-El-Malek <j...@chromium.org> wrote:

The combination of descriptive class/method names, 80 character limits and namespaces for major modules' API makes wrapping excessive.

Looking at the changes for "using content::XYZ" so far, I don't agree that there is lots of excessive wrapping being reduced.

Even if that occurs in some cases, it's not clear whether that should make the policy "use these in cases where it dramatically reduces wrapping" or "use these across all instances of a namespace".  I know in IM you argued that the latter is easier to enforce consistently and avoids "inertia" problems, but I don't see why that argument applies to, say, content:: but not to all other uses of qualifiers.

On Thu, Jan 5, 2012 at 3:27 PM, Gary Kacmarcik <gar...@chromium.org> wrote:

I interpreted this reference to the style guide as a counterpoint to your earlier comment:

We don't seem to have a definitive policy on when "using" directives can be used, and I'd like to try and reach consensus.

I've always thought that we did in fact have a well-defined policy WRT the "using" directive (ie: the one mentioned in style guide).

If the intent of my comment was unclear, even when I immediately referred to the existing policies afterwards, then I apologize.  Perhaps "I don't think our policy on using directives is explicit enough" would have been clearer language.

The current "use them only in .cc files and only when they improve readability" policy seems perfectly fine to me.  I haven't seem places where it's being abused.

There is no "only when they improve readability" in the current policy.  It doesn't appear in the Google style guide.  Furthermore, part of my argument is that it's not at all obvious whether things like "use directives on all instances of objects in the Content API" actually improve readability or not.  I bring this up now because usage of content:: objects is going to be pervasive across Chromium code and thus the current trend would likewise make these directives pervasive.  I would like us to be explicit about whether these sorts of cases are a net win.

On Thu, Jan 5, 2012 at 3:27 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

I don't think the incremental consistency we'd gain by legislating a hard rule offsets the readability lost in situations that use lots of functions from another namespace.

The rule could very well be one that allows the sorts of uses you describe.  I think it is worth adding clarity here because "don't use ever" and "use when there is a major impact on line wrapping" are distinct from each other, but they're also both distinct from "users of APIs X and Y should use using directives on every instance of API access".  If we want to allow the latter, we should say it, and be explicit about the APIs covered.

Personally I would be happy to modify my viewpoint from "just ban using outright" to "ban using except with gmock or in other exceptional cases where its absence would inherently make the API extremely cumbersome to use".  I also am happy for us to continue allowing namespace aliasing, which can be useful for things like the Autofill code.

PK

[John Abd-El-Malek]

<snip thread, sorry but I don't have time to reply to each point>

The Google C++ Style Guide is pretty explicit about what it allows, and what it doesn't. Given that:

-usage of "using" in cc files is explicitly called out as ok

-there is precedent for doing this for a module's well defined API (WebKit)

-Linus' general rule of thumb that Chrome "shouldn't diverge from standard Google engineering practices unless there is a very good reason and consensus that the divergence is needed"

I fail to see a very good reason to disallow something which the style guide clearly allow.

<This will be my last email to the thread. If you want we can discuss in person, and also after Darin weights in with the WebKit api justification.>

[Brett Wilson]

At the begining, a large portion of Chrome code did "using
std::string;" or "using std::vector;" at the top of their files, and
then there would be a lot of naked strings and vectors around. Except
only part of the team did this, and it was pretty random.

I think mostly due to Peter and my OCD, the project converged on
always writing "std::" for STL stuff. There's no written rule, but it
seems very out of place to see "vector<string>" and people implicitly
enforce this. I think converging on a consistent usage here has been
good for everybody, and the style guide encourages consistency over
almost everything else.

Since we've been adding stuff to the base namespace, I've been
encouraging people to explicitly use "base::" where they use such
objects. The main exceptions are where we've converted existing code,
and if a file uses some base type in a million places, you don't want
to go through and change each place individually (Darin did this when
he put Time in the base namespace).

I never saw the point of the "using" rule WebKit namespace, and I
usually resented doing this when asked to. What's the point of having
a namespace when all callers are required to say "using <that
namespace>"? It seems like all this does is create stupid cases like
the 86 lines of code at the beginning of render_view_impl.cc where
every single WebKit type is manually listed with using directives.

If I was doing the content refactoring, I would do the same thing as
we did for the base namespaces. Generally use them, but us "using" for
cases where doing so would make the refactoring more painful.

Brett

[Darin Fisher]

On Thu, Jan 5, 2012 at 4:14 PM, Brett Wilson <bre...@chromium.org> wrote:

At the begining, a large portion of Chrome code did "using
std::string;" or "using std::vector;" at the top of their files, and
then there would be a lot of naked strings and vectors around. Except
only part of the team did this, and it was pretty random.

I think mostly due to Peter and my OCD, the project converged on
always writing "std::" for STL stuff. There's no written rule, but it
seems very out of place to see "vector<string>" and people implicitly
enforce this. I think converging on a consistent usage here has been
good for everybody, and the style guide encourages consistency over
almost everything else.

Since we've been adding stuff to the base namespace, I've been
encouraging people to explicitly use "base::" where they use such
objects. The main exceptions are where we've converted existing code,
and if a file uses some base type in a million places, you don't want
to go through and change each place individually (Darin did this when
he put Time in the base namespace).

I never saw the point of the "using" rule WebKit namespace, and I
usually resented doing this when asked to. What's the point of having
a namespace when all callers are required to say "using <that
namespace>"? It seems like all this does is create stupid cases like
the 86 lines of code at the beginning of render_view_impl.cc where
every single WebKit type is manually listed with using directives.

I agree this is not nice.  I did this because there were WebFoo types outside of the

WebKit API, and I wanted to avoid typename collisions if we only had the Web-prefix.

I also find typing WebKit::WebFoo everywhere to be painful and an eyesore.  The

eyesore at the top of the file is not as bad as the eyesore at call-sites.

In hindsight, maybe I should have chosen a better naming scheme.  WkFoo instead

without any namespace might have been better? Or, maybe webkit::Foo.  This latter

option was discounted because it would have led to some annoying collisions with

WebCore type names :-/

 

If I was doing the content refactoring, I would do the same thing as
we did for the base namespaces. Generally use them, but us "using" for
cases where doing so would make the refactoring more painful.

Without having studied a lot of example code (which weakens my opinion), I'm also

supportive of just typing content::Foo in src/chrome.  Unlike with the WebKit prefix,

it doesn't seem quite as noisy.

That said, in src/content, I find some of the using content::Foo to be helpful as we

ultimately want to move all of src/content into the content namespace.  The using

directives help us save work while some files are still not yet in the content namespace.

-Darin

[Peter Kasting]

On Thu, Jan 5, 2012 at 5:06 PM, Darin Fisher <da...@chromium.org> wrote:

I did this because there were WebFoo types outside of the

WebKit API,

Here "did this" means "added the namespace, as opposed to having no namespace and just 'WebFoo'", right?

 

I also find typing WebKit::WebFoo everywhere to be painful and an eyesore.  The

eyesore at the top of the file is not as bad as the eyesore at call-sites.

So is there actually a rule that All Callers Shall Use "using WebKit::WebFoo"?  And if so, is that because it's easier to enforce that than to make judgment calls about which cases are eyesores?  Or perhaps because for you saving even one call-site usage is worth it?

Or is there not actually this rule?

I'm asking because I can understand the argument you're making but I'm not precisely sure how we get into the world of always using the directives at all times.  Understanding this would help me figure out how to relate the similar arguments others have made on this thread for "allow these directives if it really aids readability" to what we should do with e.g. the content:: namespace.

That said, in src/content, I find some of the using content::Foo to be helpful as we

ultimately want to move all of src/content into the content namespace.  The using

directives help us save work while some files are still not yet in the content namespace.

I think it's OK to make exceptions or do things differently to ease big transitions like this, especially when the people involved are as responsible as John is.  Regardless of what other decision we make I don't have any opposition to at least using some of these temporarily if it makes life better.

PK

[Darin Fisher]

On Thu, Jan 5, 2012 at 5:13 PM, Peter Kasting <pkas...@google.com> wrote:

On Thu, Jan 5, 2012 at 5:06 PM, Darin Fisher <da...@chromium.org> wrote:

I did this because there were WebFoo types outside of the

WebKit API,

Here "did this" means "added the namespace, as opposed to having no namespace and just 'WebFoo'", right?

 

Yes, exactly.

 

I also find typing WebKit::WebFoo everywhere to be painful and an eyesore.  The

eyesore at the top of the file is not as bad as the eyesore at call-sites.

So is there actually a rule that All Callers Shall Use "using WebKit::WebFoo"?  And if so, is that because it's easier to enforce that than to make judgment calls about which cases are eyesores?  Or perhaps because for you saving even one call-site usage is worth it?

Or is there not actually this rule?

There is no firm rule.  However, if I see a case where WebKit::WebFoo is being used and we already have using directives in the .cc file, then I typically suggest adding another using directive.  Or, if I see a case where readability would be improved greatly (less line breaking) by adding the using directive, then I suggest doing so.

Honestly, I think a good solution for the WebKit namespace would be to import the entire namespace.  If we hit collisions with ::WebFoo, then we can just type "::WebFoo" where needed.  I never did this because the style guide forbids it, but I kinda feel like it might be a reasonable exception to the rule.

 

I'm asking because I can understand the argument you're making but I'm not precisely sure how we get into the world of always using the directives at all times.  Understanding this would help me figure out how to relate the similar arguments others have made on this thread for "allow these directives if it really aids readability" to what we should do with e.g. the content:: namespace.

Makes sense.  I'm not sure if they are 1:1.  The WebKit API uses WebKit style for instance.  (WebKit style is to do "using namespace Foo" in .cpp files by the way.)

I've been happy with base::Foo because it seems to help readability in Chromium.  You can see at a glance your dependencies on the base module.  I feel happy about content::Foo for the same reason.

 

That said, in src/content, I find some of the using content::Foo to be helpful as we

ultimately want to move all of src/content into the content namespace.  The using

directives help us save work while some files are still not yet in the content namespace.

I think it's OK to make exceptions or do things differently to ease big transitions like this, especially when the people involved are as responsible as John is.  Regardless of what other decision we make I don't have any opposition to at least using some of these temporarily if it makes life better.

Perhaps "using content::Foo" could be allowed in src/chrome at least temporarily while we complete the API conversion.  The first step for src/content/public/ as I understand it is to separate chrome and content to start enabling checkdeps restrictions on content usage.  The follow-up step is to morph the headers in src/content/public/ into a "good" API.  We're probably part-way into step 2 at this point and still completing step 1.

-Darin

PK

[Jói Sigurðsson]

TL;DR:  Please understand that if we depart from the Google style
guide, it will likely mean the content work will finish later, or
require more hands.

There are two sides to a decision like this: A question of what is
the right thing to do on principle, and the pragmatic question of how
each possible decision will affect the project.
On principle, I personally see no reason to depart from the Google
style guide. I think it's nice that we have evolved conventions such
as always using std:: and most often using base:: rather than using
classes from those two namespaces, but I think those are conventions
that should just evolve around what makes the most sense in each
specific case. For the content namespace, being discussed here, there
is usually no naming conflict and since we only allow using within a
.cc file or within a function or class scope, you are never very far
from finding out where a particular class comes from.

Pragmatically, if we depart from the Google style guide to something
more restrictive, it will significantly slow down the content
refactoring work. For each move of something to the Content API that
we do, this would typically add on the order of hundreds or even
thousands of lines in .cc files that potentially need to be re-wrapped
(and often entire function call blocks that need to be reformatted
differently) because they now exceed 80 characters. It's already a
significant pain to do this for .h files.

As an aside, while John has made a couple of changes lately to bring
.cc files that used content::Foo everywhere instead of using
content::Foo at the top (such as the change Peter linked to in the
initial email to this thread), this is the exception rather than the
rule. Normally we have not first updated the .cc files to use
content::Foo everywhere but rather immediately used using content::Foo
since it saves time, is allowed by the Google style guide, and does
not (IMHO) detract from readability.

Cheers,
Jói

[Peter Kasting]

On Fri, Jan 6, 2012 at 2:16 AM, Jói Sigurðsson <j...@chromium.org> wrote:

For the content namespace, being discussed here, there
is usually no naming conflict and since we only allow using within a
.cc file or within a function or class scope, you are never very far
from finding out where a particular class comes from.

I would, in fact, be worried about naming conflicts with the content namespace, as the "layer cake" nature of the Chromium code atop content suggests to me that there might end up being a number of cases with similarly-named methods in both (generally as part of the same pipeline of handling something).

Pragmatically, if we depart from the Google style guide to something
more restrictive, it will significantly slow down the content
refactoring work.  For each move of something to the Content API that
we do, this would typically add on the order of hundreds or even
thousands of lines in .cc files that potentially need to be re-wrapped
(and often entire function call blocks that need to be reformatted
differently) because they now exceed 80 characters.  It's already a
significant pain to do this for .h files.

As I've tried to say already, I explicitly don't want to make refactoring harder and I see no reason for us to enforce anything burdensome during such a major shift as this.  Please consider this thread as discussing what we want to happen in the steady state.  Before that time period, people working on these layers should do whatever is necessary in the short term to avoid wading through excessive "process".

PK

[Darin Fisher]

^^^ +1

I'm also very sympathetic to the argument that if the C++ style guide says it is OK, then it is OK.

-Darin

[Peter Kasting]

I'll attempt to summarize so far, then make suggestions.  Please correct my errors.

  * In general, a number of people feel like explicit qualification is more readable and/or more informative than a using directive.  A smaller but nonzero number don't find using directives to be inherently less readable, in part because you can easily note the qualifier by looking at the explicit directive atop the .cc file.

  * In specific cases such as using gmock, there would be a large cost to banning using directives, where the code would get much more verbose and line-wrapped.

  * While doing major refactoring work like creating the content API, using directives can be useful to keep incremental patches from becoming unwieldy.

  * For some areas of the code like autofill, use of multiply-nested qualifiers or other long qualified names could be improved by more liberal application of namespace aliasing, which our style guide allows (through its silence) but not everyone uses.  (I didn't even know C++ had this feature, so thanks to the folks who mentioned it!)

  * Namespace aliasing aside, several people believe there are cases where judicious use of directives could noticeably reduce verbosity and line-wrapping.

  * For the specific case of the WebKit API, using directives have been more common.  This is primarily due to two factors:

    (1) Since the objects inside the API have names starting with "Web..." anyway, qualified names look especially verbose and repetitive, compared with other namespaces like base or content.

    (2) From Darin's comments, if a file uses "using" for any of these objects, it's encouraged to use "using" for them all, for consistency.  This isn't a pattern I've heard of being applied elsewhere in the code.

  * For the specific case of the content API, there was a perception that the WebKit API used directives on all inclusions, and a desire to be consistent with this.  It seems like this perception is not completely accurate.

If all of the above is correct, then I have some suggestions as to possible changes, both to our code and our style guide.

(1) Amend the style guide with something like the following.  I know a couple of people have said they see no need to add to the existing style guide rules.  Personally, I feel that this, like our other additions to the Google style guide, adds helpful clarity.  Maybe others feel like this is already what our common practice is, but at least for me, I started this thread precisely because it was not clear to me what our current text allowed.

  Use 'using' directives sparingly, when they significantly enhance readability, e.g. by saving a large amount of verbose or line-wrapped code.  For example, when using gmock, using the 'testing::' qualifier everywhere would dramatically expand the code, so directives can really help.  By contrast, if there are few uses of a qualified name within a file, or the uses don't cause lots of problems with line-wrapping, explicitly qualifying names makes the origin of the name maximally apparent at the call-site.

  For code which makes use of long or nested qualifiers, it may be convenient to use namespace aliasing ("namespace x = my::long::namespace") to reduce verbosity.

  While refactoring code, using directives can help avoid large changes that merely re-qualify names and re-wrap lines everywhere.  In these cases, more liberal use of directives than normal is fine.  Once the refactoring is complete, try to bring the use of directives in line with the guidelines above.

(2) Reduce the use of "using" directives with the WebKit API in the cases where such directives don't affect a large number of places in the file, so as not to give coders the impression that these directives are standard or required for all WebKit API usage everywhere.  Specifically, I'm thinking of cases where a file may have many specific directives atop the file, where only a few of these are actually liberally used within the file.  If this reduction still results in cases where files have lots of directives, perhaps we should consider whether to make exceptions to the "no using namespace" rule, as Darin mentioned.

This is a compromise proposal, so I know it doesn't match everyone's stated points of view.  I'd appreciate feedback.

PK

[Erik Wright]

One of the points previously raised was that, when refactoring or otherwise looking for uses of a class/function, a 'using ns::Class' at the top of the file allows me to find the affected files easily.

Namespace aliasing, unfortunately, does not support this.

PK

--

[Peter Kasting]

On Fri, Jan 6, 2012 at 11:45 AM, Erik Wright <erikw...@chromium.org> wrote:

One of the points previously raised was that, when refactoring or otherwise looking for uses of a class/function, a 'using ns::Class' at the top of the file allows me to find the affected files easily.

I missed that point, so thanks.

I don't know how important this is.  It seems like to find all uses you'd need to just search for the unqualified name regardless, since not only namespace aliasing but also usage inside the defining namespace wouldn't be caught be a search for the qualified name.  (The response of "what if there are other functions named the same?" seems like it raises an issue we already have to deal with much more frequently when two classes have similarly-named functions, so having the possibility of a couple more cases due to namespaces doesn't seem like a big deal to me.)

PK

[Ilya Sherman]

On Fri, Jan 6, 2012 at 10:30 AM, Peter Kasting <pkas...@google.com> wrote:

* For some areas of the code like autofill, use of multiply-nested qualifiers or other long qualified names could be improved by more liberal application of namespace aliasing, which our style guide allows (through its silence) but not everyone uses.  (I didn't even know C++ had this feature, so thanks to the folks who mentioned it!)

Perhaps I'm just being daft, but why is namespace aliasing better for code readability than are using directives?

The specific example for Autofill was "webkit::forms::FormData", which becomes either "wf::FormData" or "FormData".  "wf::" is 4 characters longer, but IMO actually less clear.  Also, should I choose "wf", or "wkf", or "wkforms", etc.?  Will we have additional style guidelines to help standardize how aliases are named, so that they are consistent across various .cc files?

[Dominic Mazzoni]

On Fri, Jan 6, 2012 at 3:41 PM, Ilya Sherman <ishe...@chromium.org> wrote:

The specific example for Autofill was "webkit::forms::FormData", which becomes either "wf::FormData" or "FormData".  "wf::" is 4 characters longer, but IMO actually less clear.  Also, should I choose "wf", or "wkf", or "wkforms", etc.?  Will we have additional style guidelines to help standardize how aliases are named, so that they are consistent across various .cc files?

Yes, exactly. There are several advantages to only allowing "using webkit::forms::FormData" over "namespace wf" or a typedef - (1) There's no room for interpretation, there's exactly one correct way to do it, (2) a search for the fully qualified class name will find all source files, and (3) it's consistent with the Google C++ style guide.

I don't know how important this is.  It seems like to find all uses you'd need to just search for the unqualified name regardless, since not only namespace aliasing but also usage inside the defining namespace wouldn't be caught be a search for the qualified name.

But the only exceptions are the header and source file where the class is declared and defined - every other source file using that class would have the fully qualified name somewhere in the file. Google C++ code definitely has this property and I've relied on it before to do huge refactorings.

- Dominic

[Peter Kasting]

On Fri, Jan 6, 2012 at 3:52 PM, Dominic Mazzoni <dmaz...@google.com> wrote:

I don't know how important this is.  It seems like to find all uses you'd need to just search for the unqualified name regardless, since not only namespace aliasing but also usage inside the defining namespace wouldn't be caught be a search for the qualified name.

But the only exceptions are the header and source file where the class is declared and defined - every other source file using that class would have the fully qualified name somewhere in the file.

No; by "usage in the defining namespace" I was also trying to include all other source files in that namespace.  For example, in base/ , there are many .cc files, pretty much all of which do "namespace base {" atop the file.  All of these source files can use all other objects in base:: without qualifying the names.

You, Ilya, and Erik all seem to be raising objections to namespace aliasing.  I am fine with dropping the issue of namespace aliasing entirely and simply remaining silent on it.  It is so rare right now I don't think it's really necessary to write anything on it.

With regard to the other suggestions in my summary email, are there any comments?  I've had one "+1 to everything" off-thread as well as one "I still don't think we need to amend the style guide to say anything; remaining silent on all this is superior to adding yet more text to our guide".  I'd like to hear, publicly or privately, from others so I can get a sense whether amending the style guide, doing nothing, or some other course of action is perceived to be the best thing.

PK

[Ilya Sherman]

On Fri, Jan 6, 2012 at 4:43 PM, Peter Kasting <pkas...@google.com> wrote:

With regard to the other suggestions in my summary email, are there any comments?  I've had one "+1 to everything" off-thread as well as one "I still don't think we need to amend the style guide to say anything; remaining silent on all this is superior to adding yet more text to our guide".  I'd like to hear, publicly or privately, from others so I can get a sense whether amending the style guide, doing nothing, or some other course of action is perceived to be the best thing.

I'm fine with the remaining suggestions.

I think it would be great if we could also cover when to use "typedef std::vector<breakfast::Crumpet> Crumpets" vs. leaving the full name spelled out, and naming conventions for such typedefs.  If you'd prefer to do that in a separate thread, that's also fine by me.

[Peter Kasting]

On Fri, Jan 6, 2012 at 5:07 PM, Ilya Sherman <ishe...@chromium.org> wrote:

I think it would be great if we could also cover when to use "typedef std::vector<breakfast::Crumpet> Crumpets" vs. leaving the full name spelled out, and naming conventions for such typedefs.  If you'd prefer to do that in a separate thread, that's also fine by me.

I'd prefer that to be a separate thread.

I also don't think I have a coherent enough statement on that to propose any particular text, so if you really want something added, I suggest you write it :)

PK

[Dominic Mazzoni]

On Fri, Jan 6, 2012 at 4:43 PM, Peter Kasting <pkas...@google.com> wrote:

With regard to the other suggestions in my summary email, are there any comments?  I've had one "+1 to everything" off-thread as well as one "I still don't think we need to amend the style guide to say anything; remaining silent on all this is superior to adding yet more text to our guide".  I'd like to hear, publicly or privately, from others so I can get a sense whether amending the style guide, doing nothing, or some other course of action is perceived to be the best thing.

All of the other suggestions sounded great to me. +1 to amending the style guide.

- Dominic

[David Levin]

On Fri, Jan 6, 2012 at 10:30 AM, Peter Kasting <pkas...@google.com> wrote:

Issue 1: TLDR: More rules = bad :)

Everything added to a style guide is more mental tax in trying to remember it or when trying to look it up. Also, in general Chromium has followed Google internal style in a number of cases to make the transition easier for folks and have less exceptions that people have to remember.

So there should be a high bar for adding stuff imo. I'm not sure why this meets that high bar (rather than leaving it as a judgement call).

Issue 2:  TLDR: Rules that are inconsistent = bad

I find the exception for refactoring to be odd. Is there suppose to be a post refactoring step where people go through and fix the code or will refactoring put code into a state that isn't compliant with the rule for an indefinite time?

It seems odd to have a rule that will naturally be disobeyed by code (which makes the rules inconsistent from the start).  While I dislike more rules -- if there are rules, then having inconsistency around them seems even worse.

dave