Reduce number of unnamed-type-template-args violations.

Reduce number of unnamed-type-template-args violations (mostly when passing values to DCHECK(), ASSERT_EQ(), etc.), generally by naming previously-anonymous enums.  We've decided not to eliminate the warning entirely because doing so is only possible with tons of ugly static_cast<>()s in Mac code.

BUG=92247
TEST=Compiles
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99086

[Peter Kasting, 2011-08-15 17:57:36]

I converted "enum { CONST = val; }" to using "static const" where possible.  For
other cases and true enums, I added a name to the enum where possible.  For the
remaining cases, which were all due to Mac system headers, I had to cast the
enum value to the appropriate type where it was used.  Boo to Apple for writing
their headers this way.

[Nico, 2011-08-19 17:23:57]

It looks like this patch does 3 things:

1.) Name enums (the change in base)

2.) Remove enum constants in classes with "real" constants

3.) Add static_cast<>s to EXCEPT_EQ / ASSERT_EQ

I support 1, I'm ambivalent about 2, but I don't think we should be doing 3, for
these reasons:

* It makes the code more annoying to read/write
* This is allowed in c++0x, so it'll be fine there anyway (similar to how we
allow trailing commas in enum lists: That's not valid c++ either, but all
compilers accept it, and it's officially supported in c++0x)
* Since it's not natural, people will only learn about this from clang bot
failures (the other bots just silently accept this)

[Peter Kasting, 2011-08-19 18:51:13]

I trimmed this patch down to basically be things (1) and (2).  A few instances
of (3) were still necessary on MSVC since apparently naming an enum changes its
signedness, but I eliminated the largest cause of those -- the changes in
ipc_message_macros.h -- because they didn't seem to provide much benefit
(mucking with things inside a #define seems less worthwhile to me than changing
things inside a standard class definition in a header).

[Nico, 2011-08-19 20:34:23]

lgtm

http://codereview.chromium.org/7605019/diff/29035/chrome/browser/net/predicto...
File chrome/browser/net/predictor_unittest.cc (right):

http://codereview.chromium.org/7605019/diff/29035/chrome/browser/net/predicto...
chrome/browser/net/predictor_unittest.cc:267: const ListValue& referral_list) {
This indentation style is strictly worse than the previous one (if you rename
the function, you now have to reindent the param on the following name), so I'd
revert this change

http://codereview.chromium.org/7605019/diff/29035/chrome/browser/ui/webui/opt...
File chrome/browser/ui/webui/options/certificate_manager_handler.cc (right):

http://codereview.chromium.org/7605019/diff/29035/chrome/browser/ui/webui/opt...
chrome/browser/ui/webui/options/certificate_manager_handler.cc:469:
base::FundamentalValue ssl_value(!!(trust & net::CertDatabase::TRUSTED_SSL));
(I remember seeing !! for the first time in some open source project many years
ago, and it took me a month or so to figure out why it's not a no-op. I prefer
the explicit cast that was here before. But I don't feel too strongly about it.)

http://codereview.chromium.org/7605019/diff/29035/crypto/sha2_unittest.cc
File crypto/sha2_unittest.cc (right):

http://codereview.chromium.org/7605019/diff/29035/crypto/sha2_unittest.cc#new...
crypto/sha2_unittest.cc:48:
ASSERT_EQ(static_cast<size_t>(crypto::SHA256_LENGTH), output1.size());
for msvc i assume. else revert

http://codereview.chromium.org/7605019/diff/29035/ipc/file_descriptor_set_pos...
File ipc/file_descriptor_set_posix.cc (right):

http://codereview.chromium.org/7605019/diff/29035/ipc/file_descriptor_set_pos...
ipc/file_descriptor_set_posix.cc:125: DCHECK_LE(count,
static_cast<unsigned>(MAX_DESCRIPTORS_PER_MESSAGE));
for msvc i assume. else revert

http://codereview.chromium.org/7605019/diff/29035/net/base/x509_cert_types_ma...
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/7605019/diff/29035/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:50: ValueTypes value_type;
Is this IPCd? If so, can the IPC macros handle enums?

[Peter Kasting, 2011-08-19 20:52:21]

http://codereview.chromium.org/7605019/diff/29035/net/base/x509_cert_types_ma...
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/7605019/diff/29035/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:50: ValueTypes value_type;
On 2011/08/19 20:34:23, Nico wrote:
> Is this IPCd? If so, can the IPC macros handle enums?

This is declared inside a .cc file, so it's local to that file, and within this
file I don't see any IPCing going on.

[Peter Kasting, 2011-08-19 23:09:01]

Added OWNERS:
brettw: base, content, ppapi
wtc: chrome, crypto, net

[brettw, 2011-08-21 05:37:03]

I'f remove pp_errors.h from this CL. If it's important that we change it, we
should take it up separately.

http://codereview.chromium.org/7605019/diff/29035/chrome/browser/net/predicto...
File chrome/browser/net/predictor_unittest.cc (right):

http://codereview.chromium.org/7605019/diff/29035/chrome/browser/net/predicto...
chrome/browser/net/predictor_unittest.cc:267: const ListValue& referral_list) {
I usually recommend the style applied by the patch:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
(Align params to after the ( if they fit, use 4 spaces if they don't.)

http://codereview.chromium.org/7605019/diff/29035/content/common/content_noti...
File content/common/content_notification_types.h (right):

http://codereview.chromium.org/7605019/diff/29035/content/common/content_noti...
content/common/content_notification_types.h:14: enum NotificationTypes {
IMO enums should not be named with plural. This is the name of a single type
that can take a number of values, rather than defining a list of types.

In practice, this makes it look weird if I write a function taking this enum as
an argument:
  void Foo(NotificationTypes type) {
since it's only taking one type.

http://codereview.chromium.org/7605019/diff/29035/ppapi/c/pp_errors.h
File ppapi/c/pp_errors.h (right):

http://codereview.chromium.org/7605019/diff/29035/ppapi/c/pp_errors.h#newcode27
ppapi/c/pp_errors.h:27: enum ErrorCodes {
This isn't in any namespace so I don't think we want to use this generic name
here!

We've previously discussed having a named enum type here but have decided
against it so far. PPAPI error codes represent either a value from here, or a
positive value representing success (usually number of bytes read or something).
We've been worried that naming this type would encourage people to actually use
it, which would be confusing in some cases.

[Peter Kasting, 2011-08-22 03:56:41]

I'll take pp_errors.h out of this patch because the goal here is not to make
contentious changes.

That said, comments below.

http://codereview.chromium.org/7605019/diff/29035/ppapi/c/pp_errors.h
File ppapi/c/pp_errors.h (right):

http://codereview.chromium.org/7605019/diff/29035/ppapi/c/pp_errors.h#newcode27
ppapi/c/pp_errors.h:27: enum ErrorCodes {
On 2011/08/21 05:37:03, brettw wrote:
> This isn't in any namespace so I don't think we want to use this generic name
> here!

Oof.  Honestly that seems bad.  Can't you namespace this?  I feel uncomfortable
adding this sort of thing to the global namespace.

> We've been worried that naming this type would encourage people to actually
use
> it, which would be confusing in some cases.

That's also true of various other enums I name in this patch, though, such as
the content-and-chrome notification types (where using either type separately is
bad because any function which works with them needs to handle both).  The
downside of not naming the enums is that it is impossible to refer to the type
by name even when you really want to (for example, for a function which has no
return value other than one of these codes) and (until C++11) impossible to pass
these values to templates.

[brettw, 2011-08-22 04:11:08]

http://codereview.chromium.org/7605019/diff/29035/ppapi/c/pp_errors.h
File ppapi/c/pp_errors.h (right):

http://codereview.chromium.org/7605019/diff/29035/ppapi/c/pp_errors.h#newcode27
ppapi/c/pp_errors.h:27: enum ErrorCodes {
On 2011/08/22 03:56:41, Peter Kasting wrote:
> Oof.  Honestly that seems bad.  Can't you namespace this?  I feel
uncomfortable
> adding this sort of thing to the global namespace.

This is the public Pepper API which must be compilable in C mode, so no
namespaces. We normally prefix things with PP_, so if we were to name this it
would be PP_Error or something.

[wtc, 2011-08-24 01:18:53]

pkasting: thanks for the patch.

In the CL's commit message, please mention that ASSERT_EQ,
DCHECK_EQ, etc. are the templates in question.  It took me
a while to figure that out.

I agree with brettw that the enum type names should be
singular.

http://codereview.chromium.org/7605019/diff/29035/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/7605019/diff/29035/net/proxy/proxy_config.h#ne...
net/proxy/proxy_config.h:113: static const ID kInvalidConfigID;
Nit: please shorten this to kInvalidID.

[Peter Kasting, 2011-08-24 01:29:22]

http://codereview.chromium.org/7605019/diff/29035/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/7605019/diff/29035/net/proxy/proxy_config.h#ne...
net/proxy/proxy_config.h:113: static const ID kInvalidConfigID;
On 2011/08/24 01:18:53, wtc wrote:
> Nit: please shorten this to kInvalidID.

I did at first, but it turns out there's a naming conflict between that and
another "kInvalidID" that, IIRC, some Apple system headers or something define.

[brettw, 2011-08-24 20:55:55]

LGTM

[Peter Kasting, 2011-08-26 20:43:08]

New snap up.  Removed pp_errors.h.  Added to change description.  Changed plural
enum names to singular except in cases where the enum is actually a bitfield,
e.g. "enum TrustBits" in net/base/cert_database.h or "enum CertStatusFlags" in
net/base/cert_status_flags.h.

wtc: Still need LGTM from you.

[wtc, 2011-08-26 21:20:53]

LGTM on Patch Set 14.

I only reviewed the files under src/crypto and src/net.

Could you note in the CL's commit message that it is Clang
that warns about this?

I have some questions and suggested changes below.  Thanks.

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2.h
File crypto/sha2.h (right):

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2.h#newcode19
crypto/sha2.h:19: enum Constants {
Would you be willing to name this enum ClangWarnsIfUnnamed?
It'd be more self-explanatory to me.

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2_unittest.cc
File crypto/sha2_unittest.cc (right):

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2_unittest.cc#new...
crypto/sha2_unittest.cc:48:
ASSERT_EQ(static_cast<size_t>(crypto::SHA256_LENGTH), output1.size());
Is this static_cast to size_t still necessary?  It seems
that the static_cast<size_t> should allow crypto::SHA256_LENGTH
to be of an unnamed enum type.

http://codereview.chromium.org/7605019/diff/38001/net/base/cert_database_nss_...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/7605019/diff/38001/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:287: cert_db_.GetCertTrust(cert.get(),
CA_CERT));
Are these static_casts to unsigned int necessary because
CertDatabase::GetCertTrust returns unsigned int?  If so,
I'll look into changing CertDatabase::GetCertTrust to return
int.

http://codereview.chromium.org/7605019/diff/38001/net/base/x509_cert_types_ma...
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/7605019/diff/38001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:50: ValueTypes value_type;
BUG: this needs to be changed to ValueType, singular.

http://codereview.chromium.org/7605019/diff/38001/net/http/http_stream_parser.h
File net/http/http_stream_parser.h (right):

http://codereview.chromium.org/7605019/diff/38001/net/http/http_stream_parser...
net/http/http_stream_parser.h:106: static const int kMaxBufSize;
Please initialize these static const members in the header
file.  All the compilers we support can handle this now.
You can find existing usage in src/net by running this command
in the 'src' directory:

find net -name "*.h" | xargs grep "static const int"

http://codereview.chromium.org/7605019/diff/38001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/7605019/diff/38001/net/proxy/proxy_config.h#ne...
net/proxy/proxy_config.h:113: static const ID kInvalidConfigID;
Please initialize this to 0 here in this header.

[Peter Kasting, 2011-08-26 21:32:51]

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2.h
File crypto/sha2.h (right):

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2.h#newcode19
crypto/sha2.h:19: enum Constants {
On 2011/08/26 21:20:53, wtc wrote:
> Would you be willing to name this enum ClangWarnsIfUnnamed?
> It'd be more self-explanatory to me.

No, for various reasons, not least that Clang does not warn about this (because
we don't intend to re-enable the warning).

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2_unittest.cc
File crypto/sha2_unittest.cc (right):

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2_unittest.cc#new...
crypto/sha2_unittest.cc:48:
ASSERT_EQ(static_cast<size_t>(crypto::SHA256_LENGTH), output1.size());
On 2011/08/26 21:20:53, wtc wrote:
> Is this static_cast to size_t still necessary?

Yes.

> It seems
> that the static_cast<size_t> should allow crypto::SHA256_LENGTH
> to be of an unnamed enum type.

That may be true, but the point of this change is to minimize the number of
anonymous enums in general, not just to do it to fix warnings (since we're not
re-enabling the warning, this change won't make any difference to warnings).

http://codereview.chromium.org/7605019/diff/38001/net/base/cert_database_nss_...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/7605019/diff/38001/net/base/cert_database_nss_...
net/base/cert_database_nss_unittest.cc:287: cert_db_.GetCertTrust(cert.get(),
CA_CERT));
On 2011/08/26 21:20:53, wtc wrote:
> Are these static_casts to unsigned int necessary because
> CertDatabase::GetCertTrust returns unsigned int?  If so,
> I'll look into changing CertDatabase::GetCertTrust to return
> int.

MSVC seems to choose whether an enum's underlying type is signed or not based on
whether the enum is named.  Since this is technically implementation-dependent,
this is legal, if weird, but other compilers aren't guaranteed to agree.  So
your changing the signature may not result in us needing to remove the cast. 
Therefore, I'd just let it go.

http://codereview.chromium.org/7605019/diff/38001/net/base/x509_cert_types_ma...
File net/base/x509_cert_types_mac.cc (right):

http://codereview.chromium.org/7605019/diff/38001/net/base/x509_cert_types_ma...
net/base/x509_cert_types_mac.cc:50: ValueTypes value_type;
On 2011/08/26 21:20:53, wtc wrote:
> BUG: this needs to be changed to ValueType, singular.

Thanks!

http://codereview.chromium.org/7605019/diff/38001/net/http/http_stream_parser.h
File net/http/http_stream_parser.h (right):

http://codereview.chromium.org/7605019/diff/38001/net/http/http_stream_parser...
net/http/http_stream_parser.h:106: static const int kMaxBufSize;
On 2011/08/26 21:20:53, wtc wrote:
> Please initialize these static const members in the header
> file.

AFAIK this is not technically legal until C++11.  Prevailing usage across all
Chromium is to initialize static consts in a single compilation unit, which is
also better for anyone porting this (e.g. the unofficial FreeBSD, Solaris, etc.
ports).  IMO it would be much better for any other similar usage in net/ to
change to match the rest of Chromium style.

http://codereview.chromium.org/7605019/diff/38001/net/proxy/proxy_config.h
File net/proxy/proxy_config.h (right):

http://codereview.chromium.org/7605019/diff/38001/net/proxy/proxy_config.h#ne...
net/proxy/proxy_config.h:113: static const ID kInvalidConfigID;
On 2011/08/26 21:20:53, wtc wrote:
> Please initialize this to 0 here in this header.

See previous comment.

[wtc, 2011-08-26 22:30:50]

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2_unittest.cc
File crypto/sha2_unittest.cc (right):

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2_unittest.cc#new...
crypto/sha2_unittest.cc:48:
ASSERT_EQ(static_cast<size_t>(crypto::SHA256_LENGTH), output1.size());
On 2011/08/26 21:32:51, Peter Kasting wrote:
> ... the point of this change is to minimize the number of
> anonymous enums in general, not just to do it to fix warnings (since we're not
> re-enabling the warning, this change won't make any difference to warnings).

If we are not re-enabling the warning, then we should not
change some of these unnamed enum types.  Some of them
really don't need a name.

Specifically, I'd like to opt out for the following:
1. enum Constants in crypto/sha2.h.
2. enum TrustBits in net/base/cert_database.h.

http://codereview.chromium.org/7605019/diff/38001/net/http/http_stream_parser.h
File net/http/http_stream_parser.h (right):

http://codereview.chromium.org/7605019/diff/38001/net/http/http_stream_parser...
net/http/http_stream_parser.h:106: static const int kMaxBufSize;
On 2011/08/26 21:32:51, Peter Kasting wrote:
>
> AFAIK this is not technically legal until C++11.

To prepare myself for reviewing this CL, I consulted a few
books.  Effective C++ recommends that we initialize static
const int data members when declaring them, if the compiler
supports that.  Effective C++ was published before 2007.
So this must have become legal C++ before C++11.

Initializing static const int data members this way makes
the code easier to understand, and allows the compiler to
not allocate storage for the constants if no code ever
takes their addresses.

[Peter Kasting, 2011-08-26 23:00:55]

On 2011/08/26 22:30:50, wtc wrote:
> If we are not re-enabling the warning, then we should not
> change some of these unnamed enum types.  Some of them
> really don't need a name.
> 
> Specifically, I'd like to opt out for the following:
> 1. enum Constants in crypto/sha2.h.
> 2. enum TrustBits in net/base/cert_database.h.

I disagree, and that disagreement is the whole reason for this change.  There is
no advantage to making enums anonymous.  It gives no benefits.  At least for
TrustBits above naming the enum is definitely a plus.  For Constants it's more
of a wash.

I am curious why you believe that anonymous enums are in any way desirable.

>
http://codereview.chromium.org/7605019/diff/38001/net/http/http_stream_parser.h
> File net/http/http_stream_parser.h (right):
> 
>
http://codereview.chromium.org/7605019/diff/38001/net/http/http_stream_parser...
> net/http/http_stream_parser.h:106: static const int kMaxBufSize;
> On 2011/08/26 21:32:51, Peter Kasting wrote:
> >
> > AFAIK this is not technically legal until C++11.
> 
> To prepare myself for reviewing this CL, I consulted a few
> books.  Effective C++ recommends that we initialize static
> const int data members when declaring them, if the compiler
> supports that.  Effective C++ was published before 2007.
> So this must have become legal C++ before C++11.

AFAIK it is not legal in C++03, which is the last standard before C++11.  The
C++11 standards process was ongoing before 2011 and the standards committee
agreed during that process to make this legal (I read up on this discussion
quite extensively the last time this issue came up a couple years ago). 
Therefore it's not surprising that a document which postdates C++03 could
suggest this as a possibility.  And in principle, I agree that doing this is
generally better.  However, I think we should be consistent about this -- and
other "legal in C++11 but not before" -- bits in our codebase, and there are a
truly massive number of places we do this the other way, not least because we
only dropped official VS2005 support recently (and VS2005 did not support this).
 And in practice, the space and speed effects of not declaring the values here
is unnoticeable.

I would be happy to publicly support you if you made a proposal to chromium-dev
to do a systematic change that fixed all such declarations in Chromium at once
or in batches.

[Peter Kasting, 2011-08-29 20:30:36]

New snap initializes constants in the header instead of the .cc file for the
cases wtc mentioned (and in exchange he promised me he'd propose to chromium-dev
to make this be our style across the codebase).  I intend to land this patch if
the try runs succeed.

[wtc, 2011-08-29 20:44:32]

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2_unittest.cc
File crypto/sha2_unittest.cc (right):

http://codereview.chromium.org/7605019/diff/38001/crypto/sha2_unittest.cc#new...
crypto/sha2_unittest.cc:48:
ASSERT_EQ(static_cast<size_t>(crypto::SHA256_LENGTH), output1.size());
On 2011/08/26 22:30:50, wtc wrote:
>
> Specifically, I'd like to opt out for the following:
> 1. enum Constants in crypto/sha2.h.
> 2. enum TrustBits in net/base/cert_database.h.

I still want these two files (and the associated .cc files)
removed from this CL.

An unnamed enum is useful when I do not need a name for
the enumeration type.  No code will ever use the 'Constants'
or 'TrustBits' enum type names, so why name them?  The enum
type names will make people wonder if the enum type names
should be used somewhere.

[Peter Kasting, 2011-08-30 02:22:27]

New snap up.

This eliminates "enum Constants { ... }" and replaces it with static consts.  As
long as I define the constant values at the declaration site, this still
compiles (under MSVC at least).

This also eliminates "enum SomeBitfieldType { ... }" and replaces it with
"typedef uint32 SomeBitfieldType; static const SomeBitfieldType FOO = 1 << 2;"
and similar.  Unlike using an enum, this actually allow sfor the type name to be
used for function arguments and return values, and this eliminates all
static_cast<>()s because the constants have the typedefed type.  This caught a
bug in the current code where one function's declaration vs. definition reversed
the order of two adjacent "int" arguments.

This should fix all the cases wtc didn't like.

I am willing to split this change into separate pieces if necessary since
changing the cert status type touched so much code, but I'd rather not if I can
avoid it :)

[Peter Kasting, 2011-08-30 03:25:01]

There is one problem with the combination of using static consts and defining
them at their declaration points, and that is that the Linux and Mac builds then
complain if you use one of these values alone in a DCHECK_op() function, with an
"undefined reference" link error.

There are many ways to work around this: define constants in .cc files,
separately from their declarations; change DCHECK_op(A, B) to DCHECK(A op B);
make the constant otherwise part of an expression, e.g. DCHECK_op(A | 0, B);
revert to using enums for constants that surface in DCHECK()s; etc.

In general the route I've chosen is to change DCHECK_op(A, B) to DCHECK(A op B)
where necessary.

This is something worth considering when proposing to make more of Chrome use
this style.

[Peter Kasting, 2011-08-30 19:04:19]

Nico suggested another workaround, which is to go ahead and define storage for
the affected constants in some .cc file.  I'm happy to use this as an
alternative, it seems like both methods have their issues.

[wtc, 2011-08-31 19:47:27]

Peter: you are highly respected on the Chrome team.
With all due respect, I have to decline the changes
to src/crypto and src/net in this CL.

When I first reviewed this CL (Patch Set 14), it had only
31 files, and it was clear which files I had to review.
Now Patch Set 21 has grown to 98 files.

To avoid having Nico and brettw redo their review, it
would be nice to first check in the portions of Patch Set
14 that they reviewed.

[Peter Kasting, 2011-08-31 20:44:19]

I've split off everything that has changed since patch 14 except for a few cases
where I implemented fixes you asked for exactly as you'd specified.  I'll upload
those changes as individual issues to avoid creating another huge review.

I'll land what's left here once it passes the trybots as everything that remains
has already been reviewed.