Nested/Inner class usage in Chromium

[Carlos Knippschild]

I was trying to follow the C++ dos and don'ts on nested classes in this CL of mine to create a new data struct to group some metrics specific data. But it seems that's not the way things have been done so far in Chromium.

As its usage is private to NavigatorImpl I didn't think creating a new pair of .H/.CC files was the way to go. As it is a new member I had to change navigator_impl.h to forward declare the type and declare the new private member so I am unable to limit the change to the .CC file (as an un-nested class in an anonymous namespace).

I also could not follow the above mentioned guide strictly and had to use a scoped_ptr to hold the instance because creating it as a non-pointer would demand its definition to be moved to the .H as well (field has incomplete type).

So what's the general opinion on using nested classes? It seems both style guides accept it but it's just not the usual choice. Any suggestions on what could I do that would stand closer to what's been done so far?

Thanks,

Carlos.

[Ryan Sleevi]

On Fri, Oct 10, 2014 at 4:29 AM, Carlos Knippschild <car...@chromium.org> wrote:

I was trying to follow the C++ dos and don'ts on nested classes in this CL of mine to create a new data struct to group some metrics specific data. But it seems that's not the way things have been done so far in Chromium.

As its usage is private to NavigatorImpl I didn't think creating a new pair of .H/.CC files was the way to go.

From the looks of what your CL is doing, correct.

 

As it is a new member I had to change navigator_impl.h to forward declare the type and declare the new private member so I am unable to limit the change to the .CC file (as an un-nested class in an anonymous namespace).

Because it's a data member, this is correct.

 

I also could not follow the above mentioned guide strictly and had to use a scoped_ptr to hold the instance because creating it as a non-pointer would demand its definition to be moved to the .H as well (field has incomplete type).

So what's the general opinion on using nested classes? It seems both style guides accept it but it's just not the usual choice. Any suggestions on what could I do that would stand closer to what's been done so far?

Thanks,

Carlos.

I think you may be conflating two different design goals here, although I can understand why.

The first aspect is that nested classes are discouraged in particular for public interfaces. That is, if you have class Foo, having a (public) Foo::Bar nested class means that no one can use Foo::Bar without fully including the definition of Foo (via a header dependency). There is no way you can forward declare Foo::Bar.

We strongly discourage these sorts of interfaces, because we strongly encourage forward declaration as a way of reducing/eliminating header dependencies. We want to reduce header dependencies because they can greatly improve build times, by reducing the number of edges dirtied when you make any changes.

The second aspect is that we discourage as much as possible putting things in header files that don't need to be. For the same logic as above, we encourage forward declaration in headers, rather than including the other .h files. This way, when some secondary header changes, no one that depends on your header needs to be recompiled. Similarly, putting static private members in classes is equally an anti-pattern, since any time you change an implementation detail (e.g. by changing a parameter of that private static method), all of the things that depend on your header file (which are only depending on your public interface, and don't care about your privates) have to be recompiled.

For your specific case, it's a question that ultimately comes down to design taste and weighing tradeoffs.

You can do what you did in the CL - forward declare the struct, then define it in the .cc file. This has the benefit that any data members of that struct don't need to have their .h files included in your .h, only in your .cc. This is great! But it comes with the tradeoff, as you note, of forcing any allocations of your struct as a member to be done via the heap, in the .cc.

You could put your struct definition fully in your .h, if that heap allocation mattered (e.g. it was in performance sensitive code that was frequently allocating/deallocating these objects, and locality of data was an essential characteristic), but then you'd need to further work out the forward declarations and out-of-line constructor/destructor/assignment operators for your struct. This has added complexity too. Plus, any time you change your struct (which is a private data structure, thus doesn't affect anyone who depends on you), anyone who depends on you would ahve to be recompiled, because the .h was dirty.

I think what you did in the CL is fine, and represents the good default position. But I wanted to explain the WHY of the rules, lest it be seen as blanket bans one way or the other.

[Carlos Knippschild]

Ryan, thanks a lot for the thorough explanation. Really appreciated.

[Peter Kasting]

On Fri, Oct 10, 2014 at 7:53 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

You can do what you did in the CL - forward declare the struct, then define it in the .cc file. This has the benefit that any data members of that struct don't need to have their .h files included in your .h, only in your .cc. This is great! But it comes with the tradeoff, as you note, of forcing any allocations of your struct as a member to be done via the heap, in the .cc.

You could put your struct definition fully in your .h, if that heap allocation mattered (e.g. it was in performance sensitive code that was frequently allocating/deallocating these objects, and locality of data was an essential characteristic), but then you'd need to further work out the forward declarations and out-of-line constructor/destructor/assignment operators for your struct. This has added complexity too. Plus, any time you change your struct (which is a private data structure, thus doesn't affect anyone who depends on you), anyone who depends on you would ahve to be recompiled, because the .h was dirty.

Note that in general, the Google style guide tries to strike a balance here as well, e.g. "Do not replace data members with pointers just to avoid an #include" ( http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Forward_Declarations ).  In a case like this, which is about avoiding an entire struct definition and not just an #include, I tend to agree with Ryan, I just wanted to link the above as FYI.

PK