Cleanup: Better constify some strings in base.

Cleanup: Better constify some strings in base.

Fix some lint errors along the way.

Committed: https://crrev.com/073d514d518521a5c409d5037b45e0a46602c997
Cr-Commit-Position: refs/heads/master@{#300407}

[Lei Zhang, 2014-10-07 00:17:54]

thestig@chromium.org changed reviewers:
+ danakj@chromium.org

[Lei Zhang, 2014-10-07 00:17:55]

Better start getting used to base/ reviews.

[danakj, 2014-10-07 17:13:54]

I don't get why is const char foo[] better than const char*? 

And why are some places using const char* const, but others are const char foo[]
instead?

There's lots of lint/style fixes which would be nice, maybe those should be
another CL?

https://codereview.chromium.org/632103004/diff/20001/base/debug/crash_logging...
File base/debug/crash_logging_unittest.cc (right):

https://codereview.chromium.org/632103004/diff/20001/base/debug/crash_logging...
base/debug/crash_logging_unittest.cc:47: const char kTestKey[] = "test-key";
What's the benefit of this change?

https://codereview.chromium.org/632103004/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/632103004/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.h:193: explicit TraceBufferChunk(uint32 seq)
There's a bunch of changes not related to consting strings, should this be a
separate CL? Or update the patch description

https://codereview.chromium.org/632103004/diff/20001/base/prefs/overlay_user_...
File base/prefs/overlay_user_pref_store_unittest.cc (right):

https://codereview.chromium.org/632103004/diff/20001/base/prefs/overlay_user_...
base/prefs/overlay_user_pref_store_unittest.cc:26: const char
mapped_overlay_key[] = "test.per_tab.javascript_enabled";
why not const char* const also?

https://codereview.chromium.org/632103004/diff/20001/base/strings/stringprint...
File base/strings/stringprintf_unittest.cc (right):

https://codereview.chromium.org/632103004/diff/20001/base/strings/stringprint...
base/strings/stringprintf_unittest.cc:141: for (int i = 0; i < kBufLen; ++i)
this would be kBufLen - 1 then.

https://codereview.chromium.org/632103004/diff/20001/base/strings/stringprint...
base/strings/stringprintf_unittest.cc:143: src[kBufLen] = 0;
and kBufLen - 1 here. maybe we should leave it without the extra 1 in the
constant?

[Lei Zhang, 2014-10-17 21:19:38]

https://codereview.chromium.org/632103004/diff/20001/base/debug/crash_logging...
File base/debug/crash_logging_unittest.cc (right):

https://codereview.chromium.org/632103004/diff/20001/base/debug/crash_logging...
base/debug/crash_logging_unittest.cc:47: const char kTestKey[] = "test-key";
On 2014/10/07 17:13:54, danakj wrote:
> What's the benefit of this change?

To avoid the possibility of stupid errors like below:

{
  const char foo[] = "foo";
  const char* bar = "bar";
  // read-only, does not work: foo[0] = 'x';
  // read-only, does not work: bar[0] = 'x';
  // read-only, does not work: foo = NULL;
  bar = NULL;  // this works.
}

https://codereview.chromium.org/632103004/diff/20001/base/debug/trace_event_i...
File base/debug/trace_event_impl.h (right):

https://codereview.chromium.org/632103004/diff/20001/base/debug/trace_event_i...
base/debug/trace_event_impl.h:193: explicit TraceBufferChunk(uint32 seq)
On 2014/10/07 17:13:54, danakj wrote:
> There's a bunch of changes not related to consting strings, should this be a
> separate CL? Or update the patch description

I updated the CL description.

https://codereview.chromium.org/632103004/diff/20001/base/prefs/overlay_user_...
File base/prefs/overlay_user_pref_store_unittest.cc (right):

https://codereview.chromium.org/632103004/diff/20001/base/prefs/overlay_user_...
base/prefs/overlay_user_pref_store_unittest.cc:26: const char
mapped_overlay_key[] = "test.per_tab.javascript_enabled";
On 2014/10/07 17:13:54, danakj wrote:
> why not const char* const also?

const char foo[] is preferred over const char* const foo, but in some cases, it
needs to be a pointer. These don't need to be pointers.

https://codereview.chromium.org/632103004/diff/20001/base/strings/stringprint...
File base/strings/stringprintf_unittest.cc (right):

https://codereview.chromium.org/632103004/diff/20001/base/strings/stringprint...
base/strings/stringprintf_unittest.cc:141: for (int i = 0; i < kBufLen; ++i)
On 2014/10/07 17:13:54, danakj wrote:
> this would be kBufLen - 1 then.

Good catch, thanks.

https://codereview.chromium.org/632103004/diff/20001/base/strings/stringprint...
base/strings/stringprintf_unittest.cc:143: src[kBufLen] = 0;
On 2014/10/07 17:13:54, danakj wrote:
> and kBufLen - 1 here. maybe we should leave it without the extra 1 in the
> constant?

I'd like "buffer length" to actually be the length of the buffer.

[danakj, 2014-10-18 18:47:36]

LGTM

https://codereview.chromium.org/632103004/diff/20001/base/debug/crash_logging...
File base/debug/crash_logging_unittest.cc (right):

https://codereview.chromium.org/632103004/diff/20001/base/debug/crash_logging...
base/debug/crash_logging_unittest.cc:47: const char kTestKey[] = "test-key";
On 2014/10/17 21:19:38, Lei Zhang wrote:
> On 2014/10/07 17:13:54, danakj wrote:
> > What's the benefit of this change?
> 
> To avoid the possibility of stupid errors like below:
> 
> {
>   const char foo[] = "foo";
>   const char* bar = "bar";
>   // read-only, does not work: foo[0] = 'x';
>   // read-only, does not work: bar[0] = 'x';
>   // read-only, does not work: foo = NULL;
>   bar = NULL;  // this works.
> }

Oh, cool didn't realize const char[] made the variable and the contents both
const. Cool.

[Lei Zhang, 2014-10-20 18:39:27]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-10-20 18:40:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/632103004/60001

[commit-bot: I haz the power, 2014-10-21 00:44:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:44:53]

Exceeded time limit waiting for builds to trigger.

[Lei Zhang, 2014-10-21 00:48:08]

The CQ bit was checked by thestig@chromium.org

[commit-bot: I haz the power, 2014-10-21 00:50:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/632103004/60001

[commit-bot: I haz the power, 2014-10-21 03:11:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-10-21 03:13:15]

Patchset 4 (id:??) landed as
https://crrev.com/073d514d518521a5c409d5037b45e0a46602c997
Cr-Commit-Position: refs/heads/master@{#300407}