Get VS 2015 to use const int definitions

Get VS 2015 to use 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. 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.

This changed was landed in the upstream protobuf repo:
https://github.com/google/protobuf/commit/a74e912a8be1274

With this change the chrome target builds with the latest VS 2015.

R=scottmg@chromium.org, pdr@chromium.org
BUG=440500
Committed: https://crrev.com/0693ed787f6ffd2b248ef8cf2c0dc050a4e9d8b2
Cr-Commit-Position: refs/heads/master@{#357013}

[brucedawson, 2015-10-28 17:23:01]

Description was changed from

==========
Get VS 2015 Update 1 to use shared definitions

VC++ up to VS 2015 RTM does not require explicit storage allocation for
static const integers declared in classes. VS 2015 Update 1 appears to
do so - yay for compatibility.

190023419 is the version number for the Update 1 compiler so it avoids
changing the behavior for anyone who is not on the update.

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

to

==========
Get VS 2015 Update 1 to use shared definitions

VC++ up to VS 2015 RTM does not require explicit storage allocation for
static const integers declared in classes. VS 2015 Update 1 appears to
do so - yay for compatibility.

190023419 is the version number for the Update 1 compiler so it avoids
changing the behavior for anyone who is not on the update.

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

[brucedawson, 2015-10-28 17:23:02]

brucedawson@chromium.org changed reviewers:
+ thakis@chromium.org

[brucedawson, 2015-10-28 17:24:11]

This avoids missing symbol errors in VS 2015 Update 1. The version number check
ensures that previous versions of VS are not affected. Let me know what you
think, including any suggestions for making the comment precise and accurate.

[brucedawson, 2015-10-28 17:31:48]

On 2015/10/28 17:24:11, brucedawson wrote:
> This avoids missing symbol errors in VS 2015 Update 1. The version number
check
> ensures that previous versions of VS are not affected. Let me know what you
> think, including any suggestions for making the comment precise and accurate.

Fixing this revealed another instance of the same problem. I will update this CL
to include additional fixes.

[brucedawson, 2015-10-28 22:00:35]

The protobuf compiler has an exception for _MSC_VER embedded into its code
generator which also needs updating - that is now included in this CL. I changed
the version check to be VC++ 2013 or higher because VC++ has for a long time
*allowed* these const int definitions, the only recent difference is that VC++
2015 appears to occasionally *require* them.

Microsoft has indicated that the const int definitions have been allowed since
VC++ 2005, which would mean that we could remove the #ifdefs entirely, from
Chromium and from protobuf.

Microsoft has also indicated that they don't know why Update 1 is behaving
differently. This is curious, but understanding this is not required.

A protobuf issue was opened to track this and make sure that the same fix is
applied there: https://github.com/google/protobuf/issues/917

[Peter Kasting, 2015-10-28 22:21:21]

On 2015/10/28 22:00:35, brucedawson wrote:
> A protobuf issue was opened to track this and make sure that the same fix is
> applied there: https://github.com/google/protobuf/issues/917

The protobuf folks are usually pretty responsive to actual pull requests; if you
have time to turn the patch here into a pull request they'll likely merge it (or
I can convince them to, since I've worked with them before).

If you don't have time to do that, then keep me in the loop, because someday
when I get back to updating our protobuf version I'm going to need to get this
merged upstream first.

[brucedawson, 2015-10-28 23:34:30]

> The protobuf folks are usually pretty responsive to actual pull requests; if
you
> have time to turn the patch here into a pull request they'll likely merge it
(or
> I can convince them to, since I've worked with them before).

I will create a PR for protobuf once I land the change locally.

Testing with VC++ 2013, VC++ 2015 RTM, and VC++ 2015 Update 1 shows that VC++
2013 cannot handle this change, so the #ifdefs must remain. The cutoff can be
either between VC++ 2015 and VC++ 2015 RTM, or between VC++ 2015 RTM and Update
1.

[brucedawson, 2015-10-29 00:35:46]

Description was changed from

==========
Get VS 2015 Update 1 to use shared definitions

VC++ up to VS 2015 RTM does not require explicit storage allocation for
static const integers declared in classes. VS 2015 Update 1 appears to
do so - yay for compatibility.

190023419 is the version number for the Update 1 compiler so it avoids
changing the behavior for anyone who is not on the update.

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

to

==========
Get VS 2015 to use 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. 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.

The protobuf changes will need to be upstreamed after this is landed.

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

[brucedawson, 2015-10-29 23:09:43]

Description was changed from

==========
Get VS 2015 to use 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. 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.

The protobuf changes will need to be upstreamed after this is landed.

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

to

==========
Get VS 2015 to use 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. 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.

This changed was landed in the upstream protobuf repo:
https://github.com/google/protobuf/commit/a74e912a8be1274

With this change the chrome target builds with the latest VS 2015.

R=scottmg@chromium.org, pdr@chromium.org
BUG=440500
==========

[brucedawson, 2015-10-29 23:09:43]

brucedawson@chromium.org changed reviewers:
+ pdr@chromium.org, scottmg@chromium.org
- thakis@chromium.org

[brucedawson, 2015-10-29 23:13:03]

pdr@ - can you review the Color.cpp change? It's trivial.
scottmg@ - can you review the protobuf changes? It's already committed upstream.

Both changes are needed for the latest VS 2015 and should not affect earlier VS
versions.

[scottmg, 2015-10-29 23:39:21]

lgtm

[pdr., 2015-10-30 00:20:12]

On 2015/10/29 at 23:39:21, scottmg wrote:
> lgtm

Color.cpp LGTM

[brucedawson, 2015-10-30 00:21:06]

The CQ bit was checked by brucedawson@chromium.org

[commit-bot: I haz the power, 2015-10-30 00:22:16]

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

[commit-bot: I haz the power, 2015-10-30 01:28:23]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-30 01:29:55]

Patchset 7 (id:??) landed as
https://crrev.com/0693ed787f6ffd2b248ef8cf2c0dc050a4e9d8b2
Cr-Commit-Position: refs/heads/master@{#357013}

[Sébastien Marchand, 2016-01-08 19:07:14]

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

[Sébastien Marchand, 2016-01-08 19:07:14]

Hey, have you submitted a PR to the protobuf team ? Syzygy has also a dependency
to protobuf and I'm hitting some compile issues while trying to switch to
VS2015.

[Sébastien Marchand, 2016-01-08 19:13:42]

Nvm, found it: https://github.com/google/protobuf/pull/922