More const int definitions for VC++

Avoid const int definition problems

VC++ up to VS 2015 RTM does not require explicit storage allocation for
static const integers declared in classes. VS 2015 Update 1 requires
these storage definitions in some cases. In order to avoid #ifs
around the storage definitions this change switches the problematic
consts to enums, for maximum portability.

Where needed the enums have types specified.

Many previous versions of VC++ have theoretically *allowed* a
definition to supply storage, but tests on VC++ 2013 show that this
doesn't actually work correctly - it leads to duplicate definition
errors. So, enums are the only #if option.

With this change all targets build with the latest VS 2015.

See also 1422453005.

R=danakj@chromium.org,zmo@chromium.org
BUG=440500
Committed: https://crrev.com/599857c9a6a07024a6047080f91f71f801deb4f8
Cr-Commit-Position: refs/heads/master@{#357688}

[brucedawson, 2015-10-30 18:57:02]

Description was changed from

==========
More const int definitions for VC++

These are needed to get all of Chromium to build with VS 2015 Update 1.

BUG=440500
==========

to

==========
More const int definitions for VC++

VC++ up to VS 2015 RTM does not require explicit storage allocation for
static const integers declared in classes. VS 2015 Update 1 requires
these storage definitions in some cases. It's unclear exactly what
cases - simple tests work with and without the explicit storage
allocation.

Many previous versions of VC++ have theoretically *allowed* a
definition to supply storage, but tests on VC++ 2013 show that this
doesn't actually work correctly - it leads to duplicate definition
errors. So, the change is scoped to VS 2015 only.

With this change all targets build with the latest VS 2015.

See also 1422453005.

R=danakj@chromium.org,zmo@chromium.org
BUG=440500
==========

[brucedawson, 2015-10-30 18:57:02]

brucedawson@chromium.org changed reviewers:
+ danakj@chromium.org, zmo@chromium.org

[brucedawson, 2015-10-30 20:19:20]

danakj@ - can you review time_win.cc and icon_util.cc?
zmo@ - can you review fenced_allocator.cc?

[danakj, 2015-10-30 20:23:30]

Same comment applies to all cases

https://codereview.chromium.org/1428003002/diff/40001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1428003002/diff/40001/base/time/time_win.cc#n...
base/time/time_win.cc:134: // VS 2015 and above allow these definitions and in
this case require them
nit: period

https://codereview.chromium.org/1428003002/diff/40001/base/time/time_win.cc#n...
base/time/time_win.cc:136: const int64 base::Time::kQPCOverflowThreshold;
Other statics that have storage seem to be initialized at the storage site
instead of in the header. Is there a reason to not unconditionally define this
storage and initialize it here also?

This came up in the static vs enum class discussion on chromium-dev and I
thought the outcome there was to always define storage for statics:
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/cQu6D...

[brucedawson, 2015-10-30 20:34:09]

Thanks for the pointer to that discussion.

I would *love* to unconditionally declare storage, but with VC++ 2013 and
earlier this does not work. The VC++ compiler helpfully automatically declares
storage which leads to duplicate symbol link errors. I am very sad that instead
of removing #ifs I am forced to add them. We can remove the #ifs once we drop
support for VC++ 2013, whenever that happens.

Regarding where the value is assigned, the reason to assign it at the
declaration instead of at the storage definition is partially because VC++ 2013
requires that, and partially because then the value is available to the compiler
to use as a compile-time constant.

I could also change to using enums, however this change seemed like the least
invasive, and seems consistent with our use of static const int elsewhere (see,
for instance, crrev.com/1422453005 and the protobuf code generator).

[brucedawson, 2015-10-30 20:36:59]

https://codereview.chromium.org/1428003002/diff/40001/base/time/time_win.cc
File base/time/time_win.cc (right):

https://codereview.chromium.org/1428003002/diff/40001/base/time/time_win.cc#n...
base/time/time_win.cc:134: // VS 2015 and above allow these definitions and in
this case require them
On 2015/10/30 20:23:30, danakj wrote:
> nit: period

Done.

[danakj, 2015-10-30 20:40:37]

Hm, I see what you mean about the protobuf code. Though in my POV we avoid this
problem in many places that are using enums I guess already. If you're okay with
it I'd prefer you made these into enums and avoid the problem here too.

If you feel strongly against it then LGTM w/ a question about consistency.

https://codereview.chromium.org/1428003002/diff/60001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/1428003002/diff/60001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:167: #if !defined(_MSC_VER) || _MSC_VER >= 1900
Why is this !defined || VER >= 1900, but the one in base is only checking VER >=
1900?

[brucedawson, 2015-10-30 20:51:37]

I can try the enum fix. I wasn't sure if that was preferred or not. Avoiding the
#ifdefs would certainly be nice.

https://codereview.chromium.org/1428003002/diff/60001/ui/gfx/icon_util.cc
File ui/gfx/icon_util.cc (right):

https://codereview.chromium.org/1428003002/diff/60001/ui/gfx/icon_util.cc#new...
ui/gfx/icon_util.cc:167: #if !defined(_MSC_VER) || _MSC_VER >= 1900
On 2015/10/30 20:40:37, danakj wrote:
> Why is this !defined || VER >= 1900, but the one in base is only checking VER
>=
> 1900?

The one in base is in a _win only file. But, I don't actually like that
explanation. I can add the _MSC_VER check as well there.

[Peter Kasting, 2015-10-30 20:55:41]

On 2015/10/30 20:51:37, brucedawson wrote:
> I can try the enum fix. I wasn't sure if that was preferred or not. Avoiding
the
> #ifdefs would certainly be nice.

The outcome of the discussion Dana linked earlier is that if you're having
issues with this sort of thing, fix by using enums rather than #ifdefed storage.

[brucedawson, 2015-10-30 22:50:07]

I've got the enum version working (it required typed enums in some places but
was otherwise straightforward), however the style guide says that enums are
supposed to use SHOUTY_CASE which requires modifying an additional ~8 files to
update the uses. I'm not thrilled with the churn from the renaming, but enums
are otherwise clearly better.

Thoughts?

[danakj, 2015-10-30 22:54:04]

On Fri, Oct 30, 2015 at 3:50 PM, <brucedawson@chromium.org> wrote:

> I've got the enum version working (it required typed enums in some places
> but
> was otherwise straightforward), however the style guide says that enums are
> supposed to use SHOUTY_CASE which requires modifying an additional ~8
> files to
> update the uses. I'm not thrilled with the churn from the renaming, but
> enums
> are otherwise clearly better.
>
> Thoughts?
>

From that thread, enums that are a single member that is a constant can be
kCasing. They are consts written as enums. Use SHOUTY_CASE for enums with
more than one in the set.


>
>
> https://codereview.chromium.org/1428003002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brucedawson, 2015-10-30 23:10:35]

Description was changed from

==========
More const int definitions for VC++

VC++ up to VS 2015 RTM does not require explicit storage allocation for
static const integers declared in classes. VS 2015 Update 1 requires
these storage definitions in some cases. It's unclear exactly what
cases - simple tests work with and without the explicit storage
allocation.

Many previous versions of VC++ have theoretically *allowed* a
definition to supply storage, but tests on VC++ 2013 show that this
doesn't actually work correctly - it leads to duplicate definition
errors. So, the change is scoped to VS 2015 only.

With this change all targets build with the latest VS 2015.

See also 1422453005.

R=danakj@chromium.org,zmo@chromium.org
BUG=440500
==========

to

==========
Avoid const int definitions

VC++ up to VS 2015 RTM does not require explicit storage allocation for
static const integers declared in classes. VS 2015 Update 1 requires
these storage definitions in some cases. In order to avoid #ifs
around the storage definitions this change switches the problematic
consts to enums, for maximum portability.

Where needed the enums have types specified.

Many previous versions of VC++ have theoretically *allowed* a
definition to supply storage, but tests on VC++ 2013 show that this
doesn't actually work correctly - it leads to duplicate definition
errors. So, enums are the only #if option.

With this change all targets build with the latest VS 2015.

See also 1422453005.

R=danakj@chromium.org,zmo@chromium.org
BUG=440500
==========

[brucedawson, 2015-10-30 23:33:36]

> From that thread, enums that are a single member that is a constant can be
> kCasing.

Thanks for pointing that out. That makes the enum change much more tractable.
This change now gets to *remove* a #if instead of adding some. Much better.

PTAL.

[brucedawson, 2015-10-30 23:41:40]

Description was changed from

==========
Avoid const int definitions

VC++ up to VS 2015 RTM does not require explicit storage allocation for
static const integers declared in classes. VS 2015 Update 1 requires
these storage definitions in some cases. In order to avoid #ifs
around the storage definitions this change switches the problematic
consts to enums, for maximum portability.

Where needed the enums have types specified.

Many previous versions of VC++ have theoretically *allowed* a
definition to supply storage, but tests on VC++ 2013 show that this
doesn't actually work correctly - it leads to duplicate definition
errors. So, enums are the only #if option.

With this change all targets build with the latest VS 2015.

See also 1422453005.

R=danakj@chromium.org,zmo@chromium.org
BUG=440500
==========

to

==========
Avoid const int definition problems

VC++ up to VS 2015 RTM does not require explicit storage allocation for
static const integers declared in classes. VS 2015 Update 1 requires
these storage definitions in some cases. In order to avoid #ifs
around the storage definitions this change switches the problematic
consts to enums, for maximum portability.

Where needed the enums have types specified.

Many previous versions of VC++ have theoretically *allowed* a
definition to supply storage, but tests on VC++ 2013 show that this
doesn't actually work correctly - it leads to duplicate definition
errors. So, enums are the only #if option.

With this change all targets build with the latest VS 2015.

See also 1422453005.

R=danakj@chromium.org,zmo@chromium.org
BUG=440500
==========

[danakj, 2015-10-31 00:01:52]

LGTM

[Peter Kasting, 2015-10-31 00:04:37]

On 2015/10/30 22:54:04, danakj wrote:
> On Fri, Oct 30, 2015 at 3:50 PM, <mailto:brucedawson@chromium.org> wrote:
> 
> > I've got the enum version working (it required typed enums in some places
> > but
> > was otherwise straightforward), however the style guide says that enums are
> > supposed to use SHOUTY_CASE which requires modifying an additional ~8
> > files to
> > update the uses. I'm not thrilled with the churn from the renaming, but
> > enums
> > are otherwise clearly better.
> >
> > Thoughts?
> >
> 
> From that thread, enums that are a single member that is a constant can be
> kCasing. They are consts written as enums. Use SHOUTY_CASE for enums with
> more than one in the set.

This makes some logical sense, but our style guide doesn't mention it and I
didn't see it in that thread?

[danakj, 2015-10-31 00:16:00]

On Fri, Oct 30, 2015 at 5:04 PM, <pkasting@chromium.org> wrote:

> On 2015/10/30 22:54:04, danakj wrote:
>
>> On Fri, Oct 30, 2015 at 3:50 PM, <mailto:brucedawson@chromium.org> wrote:
>>
>
> > I've got the enum version working (it required typed enums in some places
>> > but
>> > was otherwise straightforward), however the style guide says that enums
>> are
>> > supposed to use SHOUTY_CASE which requires modifying an additional ~8
>> > files to
>> > update the uses. I'm not thrilled with the churn from the renaming, but
>> > enums
>> > are otherwise clearly better.
>> >
>> > Thoughts?
>> >
>>
>
>  From that thread, enums that are a single member that is a constant can be
>> kCasing. They are consts written as enums. Use SHOUTY_CASE for enums with
>> more than one in the set.
>>
>
> This makes some logical sense, but our style guide doesn't mention it and I
> didn't see it in that thread?
>

Hm, maybe that wasn't in the thread. I did link to an example like that.
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/hDqJ4KBVqog/tW69f...

It's a common practice already in chromium:
https://code.google.com/p/chromium/codesearch#search/&q=enum.*?%7B%5C%20k%5BA...
?,?%5C%20%7D;%20-file:src/third_party&sq=package:chromium&type=cs

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2015-10-31 00:18:52]

On Fri, Oct 30, 2015 at 5:15 PM, Dana Jansens <danakj@chromium.org> wrote:

> It's a common practice already in chromium:
>
https://code.google.com/p/chromium/codesearch#search/&q=enum.*?%7B%5C%20k%5BA...
> ?,?%5C%20%7D;%20-file:src/third_party&sq=package:chromium&type=cs
>

I don't know if 190 instances is common, but I don't really object.

However, it might make sense for you to do a chromium-dev thread to make
sure this is cool with everyone else and then tweak our style guide to
mention this exception, since right now its language is pretty strict about
using ALL_CAPS.

PK

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[danakj, 2015-10-31 00:24:23]

On Fri, Oct 30, 2015 at 5:18 PM, Peter Kasting <pkasting@chromium.org>
wrote:

> On Fri, Oct 30, 2015 at 5:15 PM, Dana Jansens <danakj@chromium.org> wrote:
>
>> It's a common practice already in chromium:
>>
https://code.google.com/p/chromium/codesearch#search/&q=enum.*?%7B%5C%20k%5BA...
>> ?,?%5C%20%7D;%20-file:src/third_party&sq=package:chromium&type=cs
>>
>
> I don't know if 190 instances is common, but I don't really object.
>
> However, it might make sense for you to do a chromium-dev thread to make
> sure this is cool with everyone else and then tweak our style guide to
> mention this exception, since right now its language is pretty strict about
> using ALL_CAPS.
>

Ya it's a good idea, I'll do that next week. Thanks.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brucedawson, 2015-11-02 18:47:28]

> >> It's a common practice already in chromium:
> >>
>
https://code.google.com/p/chromium/codesearch#search/&q=enum.*?%7B%5C%20k%5BA...
> >> ?,?%5C%20%7D;%20-file:src/third_party&sq=package:chromium&type=cs

I see that the non-shouty-case naming is used in some cases where 
  enum DeepCopyHints { kNoHints = 0, kObjectIsShallow = 1 };

Since this is a breaking change (albeit for a compiler we don't yet support)
does it seem reasonable to submit the change as is and then rename the enums if
the votes go against the proposal?

[danakj, 2015-11-02 18:50:03]

On Mon, Nov 2, 2015 at 10:47 AM, <brucedawson@chromium.org> wrote:

> >> It's a common practice already in chromium:
>> >>
>>
>
>
>
https://code.google.com/p/chromium/codesearch#search/&q=enum.*?%7B%5C%20k%5BA...
>
>> >> ?,?%5C%20%7D;%20-file:src/third_party&sq=package:chromium&type=cs
>>
>
> I see that the non-shouty-case naming is used in some cases where
>   enum DeepCopyHints { kNoHints = 0, kObjectIsShallow = 1 };
>
> Since this is a breaking change (albeit for a compiler we don't yet
> support)
> does it seem reasonable to submit the change as is and then rename the
> enums if
> the votes go against the proposal?
>

I think so.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jbauman, 2015-11-03 22:29:11]

jbauman@chromium.org changed reviewers:
+ jbauman@chromium.org

[jbauman, 2015-11-03 22:29:11]

lgtm

[brucedawson, 2015-11-03 22:45:30]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-11-03 22:46:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1428003002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1428003002/80001

[commit-bot: I haz the power, 2015-11-04 00:49:07]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-11-04 00:49:48]

Patchset 5 (id:??) landed as
https://crrev.com/599857c9a6a07024a6047080f91f71f801deb4f8
Cr-Commit-Position: refs/heads/master@{#357688}