Inlining function definitions in tests?

[Ilya Sherman]

So, our style guide very clearly says not to define functions within class declarations, even in classes local to a single .cc file, because that's an implicit instruction to the compiler to inline the function [1].  At the same time, I've noticed that lots -- possibly even most -- of our test files go ahead and define methods for test-only classes as part of the declaration.

[1] http://www.chromium.org/developers/coding-style#TOC-Inline-functions

How should we reconcile these two facts?  Should we go through and convert a bunch of the existing test files to separate method declarations and definitions?  Should we let sleeping dogs lie, but aim to follow the style guide in new test code?  Should we update the style guide to make an exception for test files?

FWIW, my personal preference would be option (3), i.e. updating the style guide to reflect the predominant style that's actually used in our codebase.

Thanks,

~ilya

(This post brought to you by questions raised in [ https://codereview.chromium.org/19820004/ ].)

[Peter Kasting]

(Reply re-pasted here in correct thread.)

On Tue, Jul 23, 2013 at 4:11 PM, Ilya Sherman <ishe...@chromium.org> wrote:

How should we reconcile these two facts?  Should we go through and convert a bunch of the existing test files to separate method declarations and definitions?  Should we let sleeping dogs lie, but aim to follow the style guide in new test code?  Should we update the style guide to make an exception for test files?

(Given Ilya's codereview reference, my opinion here should not be surprising.)

We should handle it like most other style violations: it's not worth someone's time to explicitly go find and fix these just for fixing them's sake, but if you're touching the file anyway, go ahead and fix any style problems, preferably in a separate change from any functional changes to make review easy.

Regarding the value of this specific rule, I very much agree with the style guide that code -- including and perhaps even _especially_ test code, given how much boilerplate is often mixed in with the few bits of functionality -- is easier to read and understand when declarations and definitions are separate.  I have never regretted making this change to a test file.  I would be OK with doing away with this rule if it didn't seem to buy anything, but given that I think it's valuable, I'm not happy about the idea of not asking for it to be applied consistently.

Historically, it's worth noting that the rule was phrased the way it is precisely so as to emphasize that it applies to test files.

PK

[Elliot Glaysher (Chromium)]

For a little bit of historical perspective, my recollection is that we have prohibitions on inlining was because of rampant inlining of constructors, destructors and heavy methods in headers. This bloated our binaries and slowed down our compiles as we did the same work over and over again in different translation units. These rules come out of the binary size/compile time cleanup.

Regarding the value of this specific rule, I very much agree with the style guide that code -- including and perhaps even _especially_ test code, given how much boilerplate is often mixed in with the few bits of functionality -- is easier to read and understand when declarations and definitions are separate.  I have never regretted making this change to a test file.  I would be OK with doing away with this rule if it didn't seem to buy anything, but given that I think it's valuable, I'm not happy about the idea of not asking for it to be applied consistently.

That said, I agree with the broad strokes of this from a pure aesthetic point of view. I do find a separation of a separation of declarations and definitions to be easier to read in general.

-- Elliot

[Thiago Farina]

I agree with Ilya, the separation is pretty good when it's between
header (declaration) and source (definition/implementation), but not
(IMO) in test file (read source file). Because you are basically
typing the same boilerplate twice (for how much benefit?).

I can be convinced otherwise though ;)

--
Thiago

[Albert J. Wong (王重傑)]

On Tue, Jul 23, 2013 at 5:09 PM, Thiago Farina <tfa...@chromium.org> wrote:

On Tue, Jul 23, 2013 at 8:58 PM, Elliot Glaysher (Chromium)
<e...@chromium.org> wrote:
> For a little bit of historical perspective, my recollection is that we have
> prohibitions on inlining was because of rampant inlining of constructors,
> destructors and heavy methods in headers. This bloated our binaries and
> slowed down our compiles as we did the same work over and over again in
> different translation units. These rules come out of the binary size/compile
> time cleanup.

That is my recollection as well that the "no-inline" rule was largely a codegen driven decision.

 

>
>> Regarding the value of this specific rule, I very much agree with the
>> style guide that code -- including and perhaps even _especially_ test code,
>> given how much boilerplate is often mixed in with the few bits of
>> functionality -- is easier to read and understand when declarations and
>> definitions are separate.  I have never regretted making this change to a
>> test file.  I would be OK with doing away with this rule if it didn't seem
>> to buy anything, but given that I think it's valuable, I'm not happy about
>> the idea of not asking for it to be applied consistently.

I actually land pretty strongly on the side of inlining the test fixture code inside unittests.  From a consistency perspective, the prevailing convention is to inline (did a quick sanity check by examining about 40 random test files...only 3 had definitions separated out).

-Albert

> That said, I agree with the broad strokes of this from a pure aesthetic
> point of view. I do find a separation of a separation of declarations and
> definitions to be easier to read in general.
>

I agree with Ilya, the separation is pretty good when it's between
header (declaration) and source (definition/implementation), but not
(IMO) in test file (read source file). Because you are basically
typing the same boilerplate twice (for how much benefit?).

I can be convinced otherwise though ;)

--
Thiago

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

[Peter Kasting]

On Tue, Jul 23, 2013 at 5:22 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

I actually land pretty strongly on the side of inlining the test fixture code inside unittests.

Meaning, change the style guide to specifically mandate that this happens?

Also, you mentioned consistency; I assume you like this for non-consistency reasons as well, but you didn't detail that.

PK

[Paweł Hajdan, Jr.]

On Tue, Jul 23, 2013 at 5:22 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

I actually land pretty strongly on the side of inlining the test fixture code inside unittests.  From a consistency perspective, the prevailing convention is to inline (did a quick sanity check by examining about 40 random test files...only 3 had definitions separated out).

+1

Note that in most cases the method bodies are small, and are arguably easier to read inline. With lots of long methods I'd separate out the implementation, leaving the judgment up to the change author and reviewer.

If a change in the style guide is needed for this I second such a change.

Paweł

[Ryan Sleevi]

+1 to updating the style guide.

I don't think the (Chromium-specific) style guide providing guidance
on this provides significant benefit to readability - as Pawel points
out, there are times (most, in my experience) where it's more readable
to inline in the class body. The codegen argument only applied to
headers.

[Peter Kasting]

On Tue, Jul 23, 2013 at 5:43 PM, Paweł Hajdan, Jr. <phajd...@chromium.org> wrote:

Note that in most cases the method bodies are small, and are arguably easier to read inline. With lots of long methods I'd separate out the implementation, leaving the judgment up to the change author and reviewer.

:/ This sort of rule is extremely difficult to convey.  I'm not even sure what your standards for "lots" and "long" would be.  For example, consider some of the code in the review that actually triggered this thread:

* In https://codereview.chromium.org/19820004/diff/15002/chrome/browser/autofill/autofill_interactive_uitest.cc , one de-inlined class contained 12 functions totaling approximately 135 LOC.  Only two of these functions were overrides.

* In https://codereview.chromium.org/19820004/diff/15002/chrome/browser/media/chrome_media_stream_infobar_browsertest.cc , one de-inlined class contained 9 functions totaling approximately 120 LOC.  Only one of these functions was an override.

Would you consider these cases where one should inline, or de-inline?  In my own reading the code, I had a difficult time deciphering what methods were actually available on these classes (i.e. picking out the API from the larger stream of symbols), but perhaps that is only me.

PK

[Albert J. Wong (王重傑)]

On Tue, Jul 23, 2013 at 5:24 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Jul 23, 2013 at 5:22 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

I actually land pretty strongly on the side of inlining the test fixture code inside unittests.

Meaning, change the style guide to specifically mandate that this happens?

:-/  If we had to codify it then yes. However, this honestly doesn't feel like it's had a drag on readability enough over the years to warrant lengthening the style guide. There is a cost to length of style guide as well.

Also, you mentioned consistency; I assume you like this for non-consistency reasons as well, but you didn't detail that.

Yeah, I left it out because it effectively comes down to "but it works better for my particular workflow."  Consistency felt like the more relevant argument.

But if you want details, to me a unittest file is a self-contained unit and the test fixture is often not used like a class.

All fixtures follow a very specific API formula of having some combination of Constructor, Destructor, SetUp or TearDown methods.  Fixture state is exposed to TEST_Fs as protected member variables (also a style guide violation).  You never manually instantiate a fixture or even get a reference to an object having that type.  Because of this, I find it rarely useful to define the "API" of a fixture class.  Really, a test fixture class, when combined with TEST_F is an (ab)use of the object system to create a resettable symbol environment for executing tests that doesn't require restarting the process.  All the member variables and methods could be served just as well with file-scoped equivalents if there was a "reset" button.

Digression: this is also why I dislike test harnesses that inherit from testing::Test; it's mixing something that's acting like a symbol context with something that's acting like an object.

That's mostly an argument for why conventions that make sense for normal classes don't necessarily make sense for fixtures.  As for actual readability degradation, if you think of a source file as a piece of technical writing, then in a unittest the main content is in the TEST_F.  The more lines of non-TEST_F code there are, the less clear the test is IMO.  Adding extra lines of boilerplate feels like it's distracting from the main thrust of the file.

So my position is more lines & division of definition is useful if the abstraction buys us some flexibility or good behavior. However, I believe we rarely (never?) want to define the "API" of a test fixture since we only interact with the object as if all the symbols are effectively globals. Compiler inlining doesn't matter here. The division of definition just adds more lines of code and causes more LoC churn when updating signatures, etc. Thus, the weight of the rule doesn't seem to buy us enough benefit in this case.

Having said all that, in the end it's an aesthetics opinion which is hard to really defend. That why I think consistency is the more useful point.

Note also that examples in the official GoogleTest documentation all inline the functions.  Internal Google code (where this whole coding convention originated) also usually inlines text fixture implementations.

-Albert

[Peter Kasting]

Thanks for this reply.  I'm not sure I followed everything perfectly, but it was very useful.

On Tue, Jul 23, 2013 at 5:59 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

All fixtures follow a very specific API formula of having some combination of Constructor, Destructor, SetUp or TearDown methods.  Fixture state is exposed to TEST_Fs as protected member variables (also a style guide violation).

What about classes that have methods that do non-trivial work, i.e. something other than just increment some members, call through to the underlying class being tested, or provide simple access to state?  Are those cases where you feel differently?

 

Digression: this is also why I dislike test harnesses that inherit from testing::Test; it's mixing something that's acting like a symbol context with something that's acting like an object.

I'm not clear on what you're calling out here.  I assume "harnesses" means something different than "fixtures", i.e. you're not complaining about all the classes that inherit from testing::Test to do SetUp() and TearDown().  Are you talking about things like InProcessBrowserTest or something?

 

As for actual readability degradation, if you think of a source file as a piece of technical writing, then in a unittest the main content is in the TEST_F.  The more lines of non-TEST_F code there are, the less clear the test is IMO.  Adding extra lines of boilerplate feels like it's distracting from the main thrust of the file.

FWIW, this is part of why I always put divider lines between the test support structure and the actual tests.  If you want to read the tests, this makes it easy to find them, and the presence or absence of a separated class definition above the divider seems like it has no distracting effect.  I don't know if you agree though.

I also often run into cases where the test "base class(es)" is(are) far larger than the tests.  For example, in autofill_interactive_uitest.cc (to return again to the actual CL that inspired this thread), there is just one (disabled!) TEST_F taking up ~25 lines of the 360-line file.  Everything else is support.  I'm not sure de-inlining the support classes makes a categorical difference to the "main thrust" of this test.

Everything else you said seemed clear.

PK

[Ilya Sherman]

On Tue, Jul 23, 2013 at 6:11 PM, Peter Kasting <pkas...@chromium.org> wrote:

For example, in autofill_interactive_uitest.cc (to return again to the actual CL that inspired this thread), there is just one (disabled!) TEST_F taking up ~25 lines of the 360-line file.  Everything else is support.  I'm not sure de-inlining the support classes makes a categorical difference to the "main thrust" of this test.

This specific example is not a great one, as autofill_interactive_uitest.cc is really a partial clone of autofill_browsertest.cc that was created to see if running the test under the browser tests suite was the only thing causing the test to be flaky.  Turns out it wasn't, and Chris just hasn't gotten around to doing more experimentation and/or deleting the file.

[Albert J. Wong (王重傑)]

On Tue, Jul 23, 2013 at 6:11 PM, Peter Kasting <pkas...@chromium.org> wrote:

Thanks for this reply.  I'm not sure I followed everything perfectly, but it was very useful.

On Tue, Jul 23, 2013 at 5:59 PM, Albert J. Wong (王重傑) <ajw...@chromium.org> wrote:

All fixtures follow a very specific API formula of having some combination of Constructor, Destructor, SetUp or TearDown methods.  Fixture state is exposed to TEST_Fs as protected member variables (also a style guide violation).

What about classes that have methods that do non-trivial work, i.e. something other than just increment some members, call through to the underlying class being tested, or provide simple access to state?  Are those cases where you feel differently?

Having been wading through a few hundred test files over the last few months, for me the complex tests that inlined the methods were easier to modify. That might have just been a consistency thing since I was so much more use to seeing everything contained in one fixture.

Digression: this is also why I dislike test harnesses that inherit from testing::Test; it's mixing something that's acting like a symbol context with something that's acting like an object.

I'm not clear on what you're calling out here.  I assume "harnesses" means something different than "fixtures", i.e. you're not complaining about all the classes that inherit from testing::Test to do SetUp() and TearDown().  Are you talking about things like InProcessBrowserTest or something?

Sorry, for briniging this up.  It's a digression that separate to the point. When I said harness, I was thinking of AshTestBase, RenderViewTestHarness, TokenServiceTestHarness, etc. This is a completely separate discussion though.

 

 

As for actual readability degradation, if you think of a source file as a piece of technical writing, then in a unittest the main content is in the TEST_F.  The more lines of non-TEST_F code there are, the less clear the test is IMO.  Adding extra lines of boilerplate feels like it's distracting from the main thrust of the file.

FWIW, this is part of why I always put divider lines between the test support structure and the actual tests.  If you want to read the tests, this makes it easy to find them, and the presence or absence of a separated class definition above the divider seems like it has no distracting effect.  I don't know if you agree though.

I also often run into cases where the test "base class(es)" is(are) far larger than the tests.  For example, in autofill_interactive_uitest.cc (to return again to the actual CL that inspired this thread), there is just one (disabled!) TEST_F taking up ~25 lines of the 360-line file.  Everything else is support.  I'm not sure de-inlining the support classes makes a categorical difference to the "main thrust" of this test.

Yep..I can see that.  I think this ends up being a "what fits better wit your personal workflow" discussion.  The way I happen to navigate code in my editor makes the use of dividing lines nearly useless so I tend to just not notice them.  Having extra symbols with the same name though does mess with me.

There are definitely a some anti-patterns in unittests.  Anything with huge amounts of setup probably needs to have testing utilities broken out of the test file itself IMO.

[Peter Kasting]

I'd like to turn Albert/Ryan/Paweł's comments into a concrete proposal.  (People are of course still free to offer conflicting opinions.)

The proposal is to modify the existing style guide text as well as the "C++ dos and donts" text.  The precise modification could be:

(1) Simply eliminate anything about "classes declared within a .cc file" (since that's the primary sentence affecting unit tests).  This probably isn't good enough on its own (since without it, the inlining rules would still read as applying to all code, just not as explicitly-stated), and I haven't heard anyone discuss the issue of non-test code covered by this sentence, so I'm inclined to just not muck with this.

(2) Simply exclude test code from the current guidelines, a la "NOTE: In test code, these restrictions are relaxed; feel free to define functions inline in class declarations in order to reduce the amount of boilerplate needed in test fixtures."

(3) Explicitly direct people to inline in test code (would probably require prefacing all the current text with "In non-test code" and then adding a "test code" section that mandates inlining).

(4) Peter sucks, I wanted ___________________ (and also a pony).

My read is that (2) would be most preferable.  There are some consistency arguments in favor of (3), but I also hear desire to allow for judgment calls.  Therefore I propose to consider the aforementioned folks' comments as a vote for doing (2).  If any of you would actually prefer a different option, please don't hesitate to contradict me.

PK

[Ryan Sleevi]

(5) Eliminate the section and point to
http://www.chromium.org/developers/coding-style/cpp-dos-and-donts - in
particular, http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-inlining-code

I can live with (2), but it seems like the principles that guided your
change in r126 (
https://sites.google.com/a/chromium.org/dev/system/app/pages/admin/compare?wuid=wuid:gx:17ee0e09e9aefe06&rev1=126&rev2=125
) apply here - namely, keep the style guide as short and simple as
possible, and eliminate sections that require exceptions or leave
matters to taste.

[Albert J. Wong (王重傑)]

Agreed. That section looks redundant.  We have the existing cpp-dos-and-donts 

[Albert J. Wong (王重傑)]

[ again, but with complete content. ]

On Wed, Jul 24, 2013 at 10:19 AM, Ryan Sleevi <rsl...@chromium.org> wrote:

Agreed. That section looks redundant.  We have the existing cpp-dos-and-donts that talks about this. The main Google C++ style guid ealso talks about inlining (http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Inline_Functions#Inline_Functions).

Honestly, I'm okay with status quo since the drag of this particular unclarity doesn't feel too high.  I'd also be okay with (1) or (2).  (3) sounds overkill. (4) has an invalid premise and ponies are a pain ;)

－Albert

[Peter Kasting]

Based on feedback so far, Ilya, Thiago, Albert, Paweł, and Ryan support changing the style guide; Elliot and I are opposed.  Both on a sheer-numbers basis and on the basis of who responded (e.g. Paweł has probably done as much work on tests as anyone else on the team), I think "change the style guide" wins.  (This can be reversed if suddenly a bunch more respected folks speak out against this.)

Therefore, I have made the following changes to the docs:

* The style guide section on inlining ( https://sites.google.com/a/chromium.org/dev/developers/coding-style#TOC-Inline-functions ) remains but is shorter.  The one general rule on inlining that is left provides detail not found in the Google style guide, and there is also an explicit link to the Dos And Don'ts section on inlining.

* The Dos And Don'ts section on inlining has been cleaned up slightly.  The header has changed to "Stop inlining code in headers".  Then the section has gained a final subsection on code outside of headers ( https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-and-donts#TOC-What-about-code-outside-of-headers- ).  This section basically suggests following the standard inlining guidelines for code defined in .cc files, but notes reasons why test code can be an exception to this and concludes that inlining in test files is allowed and should be a readability-based decision.

People are of course welcome to read the actual changes using the links above.

Thanks for your patience.

PK

[Ryan Sleevi]

+1

Good cleanups all around. Thanks.

[Thiago Farina]

On Wed, Jul 24, 2013 at 3:56 PM, Ryan Sleevi <rsl...@chromium.org> wrote:
> +1
>
> Good cleanups all around. Thanks.
>

Changes looks all good! Thanks Peter. I really liked that you
emphasized this: "Virtual functions should never be declared this
way.", because I have seen people discussing about this and even not
following the reviewer's recommendation of using CamelCase instead of
unix_hacker for virtual functions.

--
Thiago

[Rachel Blum]

On Wed, Jul 24, 2013 at 1:38 PM, Thiago Farina <tfa...@chromium.org> wrote:

I really liked that you
emphasized this: "Virtual functions should never be declared this
way."

I hope this means they should never be hacker_style? Because a lot of the inlined test functions are things like SetUp and TearDown - which are virtual.

 - rachel

[Peter Kasting]

The intent of the style guide line is to provide guidance about most normal scenarios.  Test code is exempted from this as discussed in the Dos and Donts doc.

In other words, in non-test code, a virtual function should indeed never be inlined.

PK

[Thiago Farina]

My understading is this is talking about this:

class Interface {
public:
virtual void MyAwesomeMethod() = 0; // or virtual void MyAwesomeMethod() {}

virtual Foo* GetFoo() = 0;
};

Rather than

class Interface {
public:
virtual void my_awesome_method() = 0;

virtual Foo* foo() = 0;
};

[Thiago Farina]

On Wed, Jul 24, 2013 at 7:08 PM, Peter Kasting <pkas...@chromium.org> wrote:
> In other words, in non-test code, a virtual function should indeed never be
> inlined.
>

Sometimes we inline, when it's empty. I don't remember if clang allows
virtual functions that return bool to be inlined too

--
Thiago

[Peter Kasting]

In general, we shouldn't inline even empty functions.  There are exceptions; see the Dos and Donts file.

PK 

[John Abd-El-Malek]

I just this thread. I agree with the comments that not-inlining in test files isn't very useful and just adds more boilerplate, so I'm glad to see that we have come to a consensus there.

However I don't buy the reasons not to do it in non-test files. I think it's also more redundant copy and paste, especially for small classes/methods. If that code is later moved to a header, which I've sen many times, I've not yet seen one example where someone didn't actually split it into a .h and .cc. As for consistency, one could go both ways (i.e. in all .cc files, test or not, we can inline methods). Lastly, regarding "class API", when a class is used in one cc file, this is usually not much of an issue as there's just one caller.

I'm not saying we need to have a hard rule against this (i.e. saying always inline, or never inline). Like all things, different people have different preferences. But it seems like we should just treat all .cc fils the same, test or non-test.

AFAIK the Google style guide doesn't address this issue, which means it's not against it. The link that I saw above is a subsection under "Header Files".

--

[Alexei Svitkine]

On Thu, Jul 25, 2013 at 1:18 AM, John Abd-El-Malek <j...@chromium.org> wrote:

I just this thread. I agree with the comments that not-inlining in test files isn't very useful and just adds more boilerplate, so I'm glad to see that we have come to a consensus there.

However I don't buy the reasons not to do it in non-test files. I think it's also more redundant copy and paste, especially for small classes/methods. If that code is later moved to a header, which I've sen many times, I've not yet seen one example where someone didn't actually split it into a .h and .cc. As for consistency, one could go both ways (i.e. in all .cc files, test or not, we can inline methods). Lastly, regarding "class API", when a class is used in one cc file, this is usually not much of an issue as there's just one caller.

I'm not saying we need to have a hard rule against this (i.e. saying always inline, or never inline). Like all things, different people have different preferences. But it seems like we should just treat all .cc fils the same, test or non-test.

+1

I think if a class is entirely in the .cc file, inlining should be allowed since it cuts down on the amount of boilerplate without the issues you get when inlining in a header (i.e. linker blow up).

-Alexei

[Peter Kasting]

On Wed, Jul 24, 2013 at 10:18 PM, John Abd-El-Malek <j...@chromium.org> wrote:

However I don't buy the reasons not to do it in non-test files. I think it's also more redundant copy and paste, especially for small classes/methods. If that code is later moved to a header, which I've sen many times, I've not yet seen one example where someone didn't actually split it into a .h and .cc. As for consistency, one could go both ways (i.e. in all .cc files, test or not, we can inline methods). Lastly, regarding "class API", when a class is used in one cc file, this is usually not much of an issue as there's just one caller.

The reason the text reads this way is because all previous discussion on this thread was specific to test files; no one else addressed the issue of non-test code, so I left the recommendations reflecting our style guide's previous stance (which was explicitly to suggest that people not inline in non-test .cc files).

Indeed, Albert's comments pretty specifically applied only to test files, what with commenting about how the framework classes in those files weren't really "classes" per se.  That sort of line of argument isn't really applicable at all to non-test files.

As for your arguments, I disagree about the "class API issue isn't a big deal" as at least for the large classes I (frequently!) encounter in the frontend code, there's often fairly extensive use of the class throughout the .cc file.  The only cases where the API doesn't matter are the cases where the class is small enough (e.g. some functor) that there isn't much of an API to speak of.  That's not the common case in the code I touch.

I'm not saying we need to have a hard rule against this (i.e. saying always inline, or never inline). Like all things, different people have different preferences.

The intent of the current wording -- being outside the style guide -- is to allow author discretion.  It's a suggestion, not a requirement.  If you want to inline code in .cc files, and you don't think doing so causes any of the potential problems listed in this section, then by all means, go for it.

But it seems like we should just treat all .cc fils the same, test or non-test.

One could also state a different consistency argument: it seems like we should treat all non-test code the same, whether it uses a .h or not.  Or perhaps we should treat all small classes the same.  Etc.

In any case, I'm prepared to make additional wording changes to the docs if people truly desire it, but for the record, I think inlining code in .cc-file-defined classes is generally a bad idea and significantly detracts from readability and maintainability.  I am opposed to saying "inlining is advised for all .cc files".

PK

[John Abd-El-Malek]

All I care about is that we don't have a hard rule, and that it's up to developer preference as I mentioned before. That also means that I wouldn't want to see changes to convert one style to another as part of cleanups or nearby changes.

 

PK

[Peter Kasting]

On Thu, Jul 25, 2013 at 4:26 PM, John Abd-El-Malek <j...@chromium.org> wrote:

That also means that I wouldn't want to see changes to convert one style to another as part of cleanups or nearby changes.

Actually I would expect it to be completely appropriate to convert one style to another in order to be consistent with surrounding or similar code -- just as it is for other stylistic choices which are left up to the coder.

PK

[John Abd-El-Malek]

To be clearer: I meant if the original author writes some code in one style, having different developers come and change it just because they prefer another style.

 

PK