Fixes for re-enabling more MSVC level 4 warnings: misc edition #1

Fixes for re-enabling more MSVC level 4 warnings: misc edition #1

This contains fixes for the following sorts of issues:
* Signedness mismatch

The problem here is that using enums to declare bitmasks results in the enum values being signed, when all consumers want to use the values in an unsigned context.  Declaring them as consts allows using the more appropriate uint32 type.

In C++11 we could use "enum class" for this, but C++11 isn't legal yet.

BUG=81439
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282419

[Peter Kasting, 2014-07-09 21:33:24]

https://codereview.chromium.org/385443002/diff/20001/content/browser/storage_...
File content/browser/storage_partition_impl_unittest.cc (right):

https://codereview.chromium.org/385443002/diff/20001/content/browser/storage_...
content/browser/storage_partition_impl_unittest.cc:61:
StoragePartition::REMOVE_DATA_MASK_WEBSQL;
This change is just to keep the order the same as where the constants were
declared.

[Peter Kasting, 2014-07-10 01:18:47]

The newest version of the patch is less to my liking than the original version,
because I could not get every compiler to compile with what ought to have been
legal C++ syntax to init the constants at their declaration point in the header.

If you can't accept the patch in this state, I can try to go back to using enums
and instead add more casts and such everywhere all these values are used. 
Neither option seems particularly appealing.

[jochen (gone - plz use gerrit), 2014-07-10 11:21:30]

lgtm

[Peter Kasting, 2014-07-10 18:28:15]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-07-10 18:29:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pkasting@chromium.org/385443002/80001

[commit-bot: I haz the power, 2014-07-10 20:30:15]

Change committed as 282419

[viettrungluu, 2014-07-10 20:41:42]

On 2014/07/10 01:18:47, Peter Kasting wrote:
> The newest version of the patch is less to my liking than the original
version,
> because I could not get every compiler to compile with what ought to have been
> legal C++ syntax to init the constants at their declaration point in the
header.

FWIW:

It's a link issue, and it's really MSVC that's at fault and exhibiting
incorrect/nonstandard behavior.

Even when declaring a static constant in the header, according to the standard
you're required to have storage for it in a single compilation unit (so that
&Foo:MY_STATIC is the same everywhere), i.e., put (e.g.) |uint32_t
Foo::MY_STATIC;| in a single .cc file. MSVC does not handle this correctly.

You tend to hit the link error when using the STL containers, etc., when you end
up with reference arguments to integral types.

You can use the (awful) STATIC_CONST_MEMBER_DEFINITION macro (in
base/compiler_specific.h) to make MSVC do the right thing. Or sometimes people
just ifdef out the out-of-class definition for MSVC.

> 
> If you can't accept the patch in this state, I can try to go back to using
enums
> and instead add more casts and such everywhere all these values are used. 
> Neither option seems particularly appealing.

[Peter Kasting, 2014-07-10 20:47:47]

On 2014/07/10 20:41:42, viettrungluu wrote:
> Even when declaring a static constant in the header, according to the standard
> you're required to have storage for it in a single compilation unit

Pedantic: According to Effective C++, this is only true if you actually take the
address -- for things where one never takes the address, it's legal to have no
definition.  That might have changed in C++11 though.

Not that that matters here; unittest code _does_ take the address of some of
these (by passing them to Bind(), which takes const refs) so we have to address
the problem.

> i.e., put (e.g.) |uint32_t
> Foo::MY_STATIC;| in a single .cc file. MSVC does not handle this correctly.

Yep.  When I did that, MSVC barfed with multiple symbol definition errors,
because it apparently auto-defined things in all modules that took the address. 
Boo to MSVC.

> You can use the (awful) STATIC_CONST_MEMBER_DEFINITION macro (in
> base/compiler_specific.h) to make MSVC do the right thing. Or sometimes people
> just ifdef out the out-of-class definition for MSVC.

Wow, that's awesomely helpful.  I think I'll check in a followup here that uses
that, because having these values in the header seems a lot more readable to me
and worth the cost of ugly macros in the .cc file.

Thanks!