Don't use const char kFoo[] in the stack place.

Don't use const char kFoo[] in the stack place.

It creates a variable and copy the value all around. Instead use pointer so the other places where the variable is used
just point to the orignal variable instead of copying the value.

BUG=None
TEST=out/Debug/base_unittests --gtest_filter=EnvironmentTest.*

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=56802

[M-A Ruel, 2010-08-19 18:36:29]

lgtm

[willchan_google.com, 2010-08-19 18:47:20]

Drive-by:

On Thu, Aug 19, 2010 at 11:25 AM, <tfarina@chromium.org> wrote:

> Reviewers: Marc-Antoine Ruel,
>
> Description:
> Don't use const char kFoo[] in the stack place.
>
> It creates a variable and copy the value all around. Instead use pointer so
> the
> other places where the variable is used
> just point to the orignal variable instead of copying the value.
>

I don't understand this description.  What do you mean it copies the value
all around?  Do you mean when it initializes the stack variable?  Otherwise,
I don't know when else the array is copied.  And it's just a 3 byte array.
 If you really want to prevent the initialization, then why not just make it
static const char kFoo[]?  Making it const char* instead makes it not a
constant anymore.  It's a non-const pointer.  strlen(kFoo) is no longer a
compile-time constant.  kFooConstantNamingConvention is used for naming
constants, so please don't use it with non-const values.


>
> BUG=None
> TEST=out/Debug/base_unittests --gtest_filter=EnvironmentTest.*
>
> Please review this at http://codereview.chromium.org/3186017/show
>
> SVN Base: http://src.chromium.org/git/chromium.git
>
> Affected files:
>  M base/environment_unittest.cc
>
>
> Index: base/environment_unittest.cc
> diff --git a/base/environment_unittest.cc b/base/environment_unittest.cc
> index
>
3d7ab3cc44b05191556459f4b630e54ac5b4f3a7..5081459ce9b47f053a2222d6cfbd98c1602f16ca
> 100644
> --- a/base/environment_unittest.cc
> +++ b/base/environment_unittest.cc
> @@ -26,8 +26,8 @@ TEST_F(EnvironmentTest, HasVar) {
>  TEST_F(EnvironmentTest, SetVar) {
>   scoped_ptr<base::Environment> env(base::Environment::Create());
>
> -  const char kFooUpper[] = "FOO";
> -  const char kFooLower[] = "foo";
> +  const char* kFooUpper = "FOO";
> +  const char* kFooLower = "foo";
>   EXPECT_TRUE(env->SetVar(kFooUpper, kFooLower));
>
>   // Now verify that the environment has the new variable.
> @@ -41,8 +41,8 @@ TEST_F(EnvironmentTest, SetVar) {
>  TEST_F(EnvironmentTest, UnSetVar) {
>   scoped_ptr<base::Environment> env(base::Environment::Create());
>
> -  const char kFooUpper[] = "FOO";
> -  const char kFooLower[] = "foo";
> +  const char* kFooUpper = "FOO";
> +  const char* kFooLower = "foo";
>   // First set some environment variable.
>   EXPECT_TRUE(env->SetVar(kFooUpper, kFooLower));
>
>
>
>

[M-A Ruel, 2010-08-19 18:52:40]

On 2010/08/19 18:47:20, willchan_google.com wrote:
> Drive-by:
> 
> On Thu, Aug 19, 2010 at 11:25 AM, <mailto:tfarina@chromium.org> wrote:
> 
> > Reviewers: Marc-Antoine Ruel,
> >
> > Description:
> > Don't use const char kFoo[] in the stack place.
> >
> > It creates a variable and copy the value all around. Instead use pointer so
> > the
> > other places where the variable is used
> > just point to the orignal variable instead of copying the value.
> >
> 
> I don't understand this description.  What do you mean it copies the value
> all around?  Do you mean when it initializes the stack variable?  Otherwise,
> I don't know when else the array is copied.  And it's just a 3 byte array.
>  If you really want to prevent the initialization, then why not just make it
> static const char kFoo[]?  Making it const char* instead makes it not a
> constant anymore.  It's a non-const pointer.  strlen(kFoo) is no longer a
> compile-time constant.  kFooConstantNamingConvention is used for naming
> constants, so please don't use it with non-const values.

By style guide, const local variables aren't encouraged so it's a non-issue.

Then, there's the copy-paste issue. It's just 4 bytes each but I'm far more
concerned by copy-pasting than the actual code.

Static local variables are a whole other subject I won't touch here.

[willchan no longer on Chromium, 2010-08-19 18:59:16]

On 2010/08/19 18:52:40, Marc-Antoine Ruel wrote:
> On 2010/08/19 18:47:20, http://willchan_google.com wrote:
> > Drive-by:
> > 
> > On Thu, Aug 19, 2010 at 11:25 AM, <mailto:tfarina@chromium.org> wrote:
> > 
> > > Reviewers: Marc-Antoine Ruel,
> > >
> > > Description:
> > > Don't use const char kFoo[] in the stack place.
> > >
> > > It creates a variable and copy the value all around. Instead use pointer
so
> > > the
> > > other places where the variable is used
> > > just point to the orignal variable instead of copying the value.
> > >
> > 
> > I don't understand this description.  What do you mean it copies the value
> > all around?  Do you mean when it initializes the stack variable?  Otherwise,
> > I don't know when else the array is copied.  And it's just a 3 byte array.
> >  If you really want to prevent the initialization, then why not just make it
> > static const char kFoo[]?  Making it const char* instead makes it not a
> > constant anymore.  It's a non-const pointer.  strlen(kFoo) is no longer a
> > compile-time constant.  kFooConstantNamingConvention is used for naming
> > constants, so please don't use it with non-const values.
> 
> By style guide, const local variables aren't encouraged so it's a non-issue.

I'm not sure what this statement means.  I guess it isn't encouraged, but I'm
not sure if it's discouraged either.  I think it's just not discussed.  But my
point was more that it's a non-const variable, and thus should not be named
using the const naming convention, since that's misleading.

> 
> Then, there's the copy-paste issue. It's just 4 bytes each but I'm far more
> concerned by copy-pasting than the actual code.

Fair enough.

> 
> Static local variables are a whole other subject I won't touch here.

Just to make sure I'm not misinterpreted here, I wasn't suggesting function
local static variables, but a function local static constant.  If you wanted to
reduce the copy-paste, then making it a file static constant is fine by me too.

[M-A Ruel, 2010-08-19 19:10:48]

On 2010/08/19 18:59:16, willchan wrote:
> On 2010/08/19 18:52:40, Marc-Antoine Ruel wrote:
> > On 2010/08/19 18:47:20, http://willchan_google.com wrote:
> > By style guide, const local variables aren't encouraged so it's a non-issue.
> 
> I'm not sure what this statement means.  I guess it isn't encouraged, but I'm
> not sure if it's discouraged either.  I think it's just not discussed.  But my
> point was more that it's a non-const variable, and thus should not be named
> using the const naming convention, since that's misleading.

I agree it's probably as bad as "int kAnswer = 42;" but I still think it's the
preferable form.
 

> > Then, there's the copy-paste issue. It's just 4 bytes each but I'm far more
> > concerned by copy-pasting than the actual code.
> 
> Fair enough.
>
> > 
> > Static local variables are a whole other subject I won't touch here.
> 
> Just to make sure I'm not misinterpreted here, I wasn't suggesting function
> local static variables, but a function local static constant.  If you wanted
to
> reduce the copy-paste, then making it a file static constant is fine by me
too.

You think MSVC is that bright and makes a difference? Eh.