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
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.)
What about e.g.
typedef std::vector<breakfast::Crumpet> Crumpets;
PK--
Never using "using" also provides a gentle discouragement to going too
apeshit with namespaces.
-scott
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--
--
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.
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 ...
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 ;)
// Forbidden -- This pollutes the namespace. using namespace foo;
// OK in .cc files. using ::foo::bar;
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.
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 latter could be discouraged but allowed when it really helps readability and line wrapping.
If we're going to be strict on no 'using' that includes gmockIf gmock merits an exception that seems to argue there are other reasons to have exceptions.
If we're going to be strict on no 'using' that includes gmockIf 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
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 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.
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.
> 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.
On Thu, Jan 5, 2012 at 3:23 PM, John Abd-El-Malek <j...@chromium.org> wrote:Yes. And of those things, it's the 80-character limit I'd get rid of.
> The combination of descriptive class/method names, 80 character limits and
> namespaces for major modules' API makes wrapping excessive.
The combination of descriptive class/method names, 80 character limits and namespaces for major modules' API makes wrapping excessive.
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).
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.
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.
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
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.
I did this because there were WebFoo types outside of theWebKit API,
I also find typing WebKit::WebFoo everywhere to be painful and an eyesore. Theeyesore at the top of the file is not as bad as the eyesore at call-sites.
That said, in src/content, I find some of the using content::Foo to be helpful as weultimately want to move all of src/content into the content namespace. The usingdirectives help us save work while some files are still not yet in the content namespace.
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 theWebKit 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. Theeyesore 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 weultimately want to move all of src/content into the content namespace. The usingdirectives 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
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
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.
--
PK
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.
* 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!)
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?
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.
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.
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 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.
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.
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).
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.