Style question: static const members vs enums

[Matthew Wolenetz]

Following http://stackoverflow.com/questions/204983/static-const-member-value-vs-member-enum-which-method-is-better-why, I'm confused when I also see relatively limited usage in Chromium of https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_specific.h&q=STATIC_CONST_MEMBER_DEFINITION%20compiler&type=cs&sq=package:chromium&l=71.

For integer constants associated with a class, do we prefer still using enums, or do we prefer using static const class members now that compilers generally do the right thing? If the latter, should we:

a) always use STATIC_CONST_MEMBER_DEFINITION in the .cc if the static const member is declared (and initialized) in the .h, or

b) only define in the .cc if the static const member is declared (and initialized) in the .h *and* the member is used by reference, and when defining use STATIC_CONST_MEMBER_DEFINITION, or 

c) (I suspect not) always define the member in the .cc if the static const member is declared (and initialized) in the .h, and only use STATIC_CONST_MEMBER_DEFINITION in that definition if the member is ever used by reference?

Maybe this is FAQ, or even in the style guides, but I couldn't find an answer searching.

Thanks!

Matt

[Dana Jansens]

On Tue, Aug 11, 2015 at 4:50 PM, Matthew Wolenetz <wole...@chromium.org> wrote:

Following http://stackoverflow.com/questions/204983/static-const-member-value-vs-member-enum-which-method-is-better-why, I'm confused when I also see relatively limited usage in Chromium of https://code.google.com/p/chromium/codesearch#chromium/src/base/compiler_specific.h&q=STATIC_CONST_MEMBER_DEFINITION%20compiler&type=cs&sq=package:chromium&l=71.

For integer constants associated with a class, do we prefer still using enums, or do we prefer using static const class members now that compilers generally do the right thing? If the latter, should we:

How generally correct are they? For example: http://stackoverflow.com/a/211180 Is that something to worry about? enum is always obviously correct here. One day we'll get constexptr.

 

a) always use STATIC_CONST_MEMBER_DEFINITION in the .cc if the static const member is declared (and initialized) in the .h, or

b) only define in the .cc if the static const member is declared (and initialized) in the .h *and* the member is used by reference, and when defining use STATIC_CONST_MEMBER_DEFINITION, or 

Does this work on all of our platforms/compilers as you'd expect?

 

c) (I suspect not) always define the member in the .cc if the static const member is declared (and initialized) in the .h, and only use STATIC_CONST_MEMBER_DEFINITION in that definition if the member is ever used by reference?

Maybe this is FAQ, or even in the style guides, but I couldn't find an answer searching.

Thanks!

Matt

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

[Peter Kasting]

On Tue, Aug 11, 2015 at 4:50 PM, Matthew Wolenetz <wole...@chromium.org> wrote:

For integer constants associated with a class, do we prefer still using enums, or do we prefer using static const class members now that compilers generally do the right thing? If the latter, should we:

If possible, use a const, or a static const member (or similar) in preference to the "enum hack".

STATIC_CONST_MEMBER_DEFINITION is generally not necessary to get things to work well, but in some cases it's still necessary to make all compilers happy.  (The symptom of this is that without it you get gcc complaining when you do one thing and MSVC complaining when you do the other.)  I've probably needed this macro in two cases total, so it's rare enough that most people don't know about it.

PK

[Nico Weber]

If you don't need to take the address, use an enum. It's simpler, works better, and I'm not sure why we even have STATIC_CONST_MEMBER_DEFINITION (declspec(selectany) has a bunch of surprising side effects).

On Tue, Aug 11, 2015 at 4:50 PM, Matthew Wolenetz <wole...@chromium.org> wrote:

--

[Nico Weber]

On Tue, Aug 11, 2015 at 5:08 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 4:50 PM, Matthew Wolenetz <wole...@chromium.org> wrote:

For integer constants associated with a class, do we prefer still using enums, or do we prefer using static const class members now that compilers generally do the right thing? If the latter, should we:

If possible, use a const, or a static const member (or similar) in preference to the "enum hack".

No, use an enum. There's nothing hacky about it, and it just works better. static consts most often end in tears, and there are few good reasons for wanting to use them. 

 

STATIC_CONST_MEMBER_DEFINITION is generally not necessary to get things to work well, but in some cases it's still necessary to make all compilers happy.  (The symptom of this is that without it you get gcc complaining when you do one thing and MSVC complaining when you do the other.)  I've probably needed this macro in two cases total, so it's rare enough that most people don't know about it.

PK

--

[Peter Kasting]

On Tue, Aug 11, 2015 at 5:09 PM, Nico Weber <tha...@chromium.org> wrote:

If you don't need to take the address, use an enum. It's simpler, works better,

I disagree with both "simpler" and "works better".

An enum is generally more lines of code.  It's not simpler in the common case where you just declare the constant and are done with it.

More importantly, an enum is generally implicitly int, but explicitly specifying the type is clearer, and it's better when it turns out you don't want an int.  Plenty of webrtc code needed larger-than-I-would-have-liked changes because I needed various constants to be size_ts and they'd been defined as enums.  (No, I'm not going to use "enum class" for that.)

A constant is semantically clear.  It reads like it means.  Using "enum" to mean "const" may be a common idiom but it's not an inherently clear one.  Much like git syntax, it only makes any sense through familiarity.

As Dana mentioned, "one day we'll get constexpr", and hopefully at that point we can agree that constexpr is better than either of the above, but until then, using actual typed constants is usually simpler, always clearer, generally more maintainable, and hopefully will more trivially convert to constexpr in the future.

PK

[Dana Jansens]

On Tue, Aug 11, 2015 at 5:14 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:09 PM, Nico Weber <tha...@chromium.org> wrote:

If you don't need to take the address, use an enum. It's simpler, works better,

I disagree with both "simpler" and "works better".

An enum is generally more lines of code.  It's not simpler in the common case where you just declare the constant and are done with it.

More importantly, an enum is generally implicitly int, but explicitly specifying the type is clearer, and it's better when it turns out you don't want an int.  Plenty of webrtc code needed larger-than-I-would-have-liked changes because I needed various constants to be size_ts and they'd been defined as enums.  (No, I'm not going to use "enum class" for that.)

A constant is semantically clear.  It reads like it means.  Using "enum" to mean "const" may be a common idiom but it's not an inherently clear one.  Much like git syntax, it only makes any sense through familiarity.

FWIW I generally like people using enum cuz I know exactly what it'll do, and you can write it all in one line, even with a type. Example: https://code.google.com/p/chromium/codesearch#chromium/src/cc/quads/draw_quad.h&l=116

But I didn't even know it works to just not make a declaration.

 

As Dana mentioned, "one day we'll get constexpr", and hopefully at that point we can agree that constexpr is better than either of the above, but until then, using actual typed constants is usually simpler, always clearer, generally more maintainable, and hopefully will more trivially convert to constexpr in the future.

PK

--

[Nico Weber]

On Tue, Aug 11, 2015 at 5:14 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:09 PM, Nico Weber <tha...@chromium.org> wrote:

If you don't need to take the address, use an enum. It's simpler, works better,

I disagree with both "simpler" and "works better".

An enum is generally more lines of code.

It's 3 lines, one of which is "};". The static const int is 2 (since you need storage).

 

  It's not simpler in the common case where you just declare the constant and are done with it.

More importantly, an enum is generally implicitly int, but explicitly specifying the type is clearer

enum foo : int { FOO = 1243; };

 

, and it's better when it turns out you don't want an int.  Plenty of webrtc code needed larger-than-I-would-have-liked changes because I needed various constants to be size_ts and they'd been defined as enums.  (No, I'm not going to use "enum class" for that.)

You don't need an enum class to define an underlying type.

 

A constant is semantically clear.  It reads like it means.

No, the "constant" wants that you define storage somewhere. An enum is how you define a semantically clear enum.

 

Using "enum" to mean "const" may be a common idiom but it's not an inherently clear one.  Much like git syntax, it only makes any sense through familiarity.

As Dana mentioned, "one day we'll get constexpr", and hopefully at that point we can agree that constexpr is better than either of the above, but until then, using actual typed constants is usually simpler,

No. It has more semantic baggage. An enum is just a constant. A static can have its address taken, it's not guaranteed to be in the header (in which case it won't be inlined and can introduce static initializers, …)

 

always clearer

Equally clear.

 

generally more maintainable

Huh?

[Sunny Sachanandani]

Reading this discussion, I just realized that I forgot to add storage for a static const member in one of my recent CLs and that wasn't caught by code review either. So my vote is for using enums.

P.S. the static const is unused at the moment so it wasn't caught by the linker.

[Peter Kasting]

On Tue, Aug 11, 2015 at 5:22 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:14 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:09 PM, Nico Weber <tha...@chromium.org> wrote:

If you don't need to take the address, use an enum. It's simpler, works better,

I disagree with both "simpler" and "works better".

An enum is generally more lines of code.

It's 3 lines, one of which is "};". The static const int is 2 (since you need storage).

You usually don't, in C++11 and onward.

The intent of the language (see various standards committee email threads on this in the C++0x days) was always to allow things like "static const size_t i = 2;" in class declarations and not require storage, but in olden days this often only worked with ints, and it didn't get properly fixed in the language until C++11.  Now that we have C++11-conformant compilers this is generally doable without storage.

  It's not simpler in the common case where you just declare the constant and are done with it.

More importantly, an enum is generally implicitly int, but explicitly specifying the type is clearer

enum foo : int { FOO = 1243; };

Fine, I withdraw that objection.  I still feel this is less readable than:

const int foo = 1243;

A constant is semantically clear.  It reads like it means.

No, the "constant" wants that you define storage somewhere. An enum is how you define a semantically clear enum.

Incorrect, see above.

PK

[James Robinson]

On Tue, Aug 11, 2015 at 5:36 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:22 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:14 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:09 PM, Nico Weber <tha...@chromium.org> wrote:

If you don't need to take the address, use an enum. It's simpler, works better,

I disagree with both "simpler" and "works better".

An enum is generally more lines of code.

It's 3 lines, one of which is "};". The static const int is 2 (since you need storage).

You usually don't, in C++11 and onward.

The intent of the language (see various standards committee email threads on this in the C++0x days) was always to allow things like "static const size_t i = 2;" in class declarations and not require storage, but in olden days this often only worked with ints, and it didn't get properly fixed in the language until C++11.  Now that we have C++11-conformant compilers this is generally doable without storage.

That's not true.  A declaration in a class still requires storage in C++11 if it is ODR-used.  The definition of ODR-used is subtle and is modified further in C++14 and there's this open issues on the standard about different cases(example http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2104) but I don't think your statement is correct in general.  I believe that currently a static const in a class declaration still needs storage in the general case.  If it's not ODR-used then it doesn't, but knowing whether something is or ever will be ODR-used is incredibly subtle - for instance if you use the value in a log statement you're almost certainly ODR-using it.

In C++11 the best way to handle this for integeral types is with an enum.  If you are targeting a future version of C++ with some set of the currently pending proposals accepted and implemented you might be able to get away without a definition in some cases, but I wouldn't bet on it.

- James

[Reid Kleckner]

On Tue, Aug 11, 2015 at 5:36 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:22 PM, Nico Weber <tha...@chromium.org> wrote:

It's 3 lines, one of which is "};". The static const int is 2 (since you need storage).

You usually don't, in C++11 and onward.

The intent of the language (see various standards committee email threads on this in the C++0x days) was always to allow things like "static const size_t i = 2;" in class declarations and not require storage, but in olden days this often only worked with ints, and it didn't get properly fixed in the language until C++11.  Now that we have C++11-conformant compilers this is generally doable without storage.

I don't believe you:

$ cat t.cpp

#include <algorithm>

struct A { static const int x = 42; };

int main() { return std::max(3, A::x); }

$ clang++ -std=c++11 t.cpp -o t && ./t

/tmp/t-829692.o: In function `main':

t.cpp:(.text+0xe): undefined reference to `A::x'

clang-3.7: error: linker command failed with exit code 1 (use -v to see invocation)

$ g++ -std=c++11 t.cpp -o t && ./t

/tmp/ccebr7C5.o: In function `main':

t.cpp:(.text+0x14): undefined reference to `A::x'

collect2: error: ld returned 1 exit status

Maybe C++11 says that this is supposed to work, but that doesn't seem to be the case for compilers used by Chromium. I think constexpr doesn't actually fix this either. :-(

It's possible that you're right about what the standard says, and that compilers for the Itanium C++ ABI never implemented it because it's an ABI break.

[Peter Kasting]

On Tue, Aug 11, 2015 at 5:47 PM, James Robinson <jam...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:36 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:22 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:14 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:09 PM, Nico Weber <tha...@chromium.org> wrote:

If you don't need to take the address, use an enum. It's simpler, works better,

I disagree with both "simpler" and "works better".

An enum is generally more lines of code.

It's 3 lines, one of which is "};". The static const int is 2 (since you need storage).

You usually don't, in C++11 and onward.

The intent of the language (see various standards committee email threads on this in the C++0x days) was always to allow things like "static const size_t i = 2;" in class declarations and not require storage, but in olden days this often only worked with ints, and it didn't get properly fixed in the language until C++11.  Now that we have C++11-conformant compilers this is generally doable without storage.

That's not true.  A declaration in a class still requires storage in C++11 if it is ODR-used.  The definition of ODR-used is subtle and is modified further in C++14 and there's this open issues on the standard about different cases(example http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2104) but I don't think your statement is correct in general.  I believe that currently a static const in a class declaration still needs storage in the general case.  If it's not ODR-used then it doesn't, but knowing whether something is or ever will be ODR-used is incredibly subtle - for instance if you use the value in a log statement you're almost certainly ODR-using it.

I'm speaking about non-ODR-used constants.  Most such constants aren't used in LOG statements and similar.  I always default to using consts instead of enums and I very rarely need to go back and add storage, because the usage of such constants is usually very simple.

This is further simplified if people apply the style guide rule to "define variables where they're used" to constants as well.  Frequently something doesn't need to be a static const class member, it can just be a local const within a function (and I ask for this where possible in my reviews).  In that case you typically avoid worrying about the above entirely.

PK

[Thiago Farina]

On Tue, Aug 11, 2015 at 9:47 PM, 'Reid Kleckner' via Chromium-dev <chromi...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:36 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:22 PM, Nico Weber <tha...@chromium.org> wrote:

It's 3 lines, one of which is "};". The static const int is 2 (since you need storage).

You usually don't, in C++11 and onward.

The intent of the language (see various standards committee email threads on this in the C++0x days) was always to allow things like "static const size_t i = 2;" in class declarations and not require storage, but in olden days this often only worked with ints, and it didn't get properly fixed in the language until C++11.  Now that we have C++11-conformant compilers this is generally doable without storage.

I don't believe you:

$ cat t.cpp

#include <algorithm>

struct A { static const int x = 42; };

 

You need this to get it compiling:

struct A { static const int x; };

const int A::x = 42; 

--

Thiago Farina

[James Robinson]

On Tue, Aug 11, 2015 at 5:58 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:47 PM, James Robinson <jam...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:36 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:22 PM, Nico Weber <tha...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:14 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Tue, Aug 11, 2015 at 5:09 PM, Nico Weber <tha...@chromium.org> wrote:

If you don't need to take the address, use an enum. It's simpler, works better,

I disagree with both "simpler" and "works better".

An enum is generally more lines of code.

It's 3 lines, one of which is "};". The static const int is 2 (since you need storage).

You usually don't, in C++11 and onward.

The intent of the language (see various standards committee email threads on this in the C++0x days) was always to allow things like "static const size_t i = 2;" in class declarations and not require storage, but in olden days this often only worked with ints, and it didn't get properly fixed in the language until C++11.  Now that we have C++11-conformant compilers this is generally doable without storage.

That's not true.  A declaration in a class still requires storage in C++11 if it is ODR-used.  The definition of ODR-used is subtle and is modified further in C++14 and there's this open issues on the standard about different cases(example http://www.open-std.org/jtc1/sc22/wg21/docs/cwg_active.html#2104) but I don't think your statement is correct in general.  I believe that currently a static const in a class declaration still needs storage in the general case.  If it's not ODR-used then it doesn't, but knowing whether something is or ever will be ODR-used is incredibly subtle - for instance if you use the value in a log statement you're almost certainly ODR-using it.

I'm speaking about non-ODR-used constants.  Most such constants aren't used in LOG statements and similar.  I always default to using consts instead of enums and I very rarely need to go back and add storage, because the usage of such constants is usually very simple.

That only works if you assume every developer who is going to use this constant knows when it is being ODR-used and when it isn't.  I think it's pretty much impossible to expect the author of a constant to know if anyone will need to ODR-use the constant anywhere down the line or for developers down the line to to check their constants to see if they are ODR-use-safe, or even to know if their code is ODR-using the variable.  It's incredibly hard to know whether a particular use is an ODR-use or not without knowing a lot of details of the language, some of which aren't pinned down yet, and knowing a lot about how the libraries we use (including the C++ standard library example earlier).  Take the std::max() example earlier - how many developers would think that using a constant as a parameter counts as ODR-using?  The answer is that of course it does, since std::max() defines its parameters as const references, except that there *might* be an exception made for the C++14 constexpr version of std::max in the future.  Or there might not, it's unknown.  Getting this wrong means that code may or may not work today but exhibits undefined behavior, or will exhibit undefined behavior down the line.

- James

[Peter Kasting]

On Tue, Aug 11, 2015 at 6:10 PM, James Robinson <jam...@google.com> wrote:

On Tue, Aug 11, 2015 at 5:58 PM, Peter Kasting <pkas...@chromium.org> wrote:

I'm speaking about non-ODR-used constants.  Most such constants aren't used in LOG statements and similar.  I always default to using consts instead of enums and I very rarely need to go back and add storage, because the usage of such constants is usually very simple.

That only works if you assume every developer who is going to use this constant knows when it is being ODR-used and when it isn't.

Unless I am mistaken about the consequences of not allocating storage, this is pretty easy: declare your const, then if it compiles on all our targets you're good, and if it does happen to be ODR-used it won't compile and you can either add storage or switch to an enum.  Is there a case where this is undefined behavior rather than a compiler error?  Because if so, then I will concede that that's a bad enough consequence to change the recommendation here.

(Of course some people feel the hassle of potentially having to fix this at all is enough to sink it, but as I said, in my experience using these constants, it's relatively rare for them to actually need storage and thus for a problem to occur at all, because usually they're used in very limited circumstances, e.g. just to do some basic initialization or math, in which case you don't end up needing to think about it.)

PK

[Nico Weber]

To sum up: Just use an enum if you don't need to take the address. No special cases to keep in mind, and it's a simple rule.

[Peter Kasting]

On Tue, Aug 11, 2015 at 6:30 PM, Nico Weber <tha...@chromium.org> wrote:

To sum up: Just use an enum if you don't need to take the address. No special cases to keep in mind, and it's a simple rule.

I agree that using an enum will mean you will never have cases where a compiler complains you didn't allocate storage, but I continue to find the const method more readable and not burdensome to use in practice.

PK

[Steve Kobes]

On Tue, Aug 11, 2015 at 6:19 PM, Peter Kasting <pkas...@chromium.org> wrote:

Unless I am mistaken about the consequences of not allocating storage, this is pretty easy: declare your const, then if it compiles on all our targets you're good, and if it does happen to be ODR-used it won't compile and you can either add storage or switch to an enum.  Is there a case where this is undefined behavior rather than a compiler error?  Because if so, then I will concede that that's a bad enough consequence to change the recommendation here.

ODR violations are undefined behavior. [http://en.cppreference.com/w/cpp/language/ub]

[Peter Kasting]

They're undefined when run, but are they things the compiler won't catch?  (There are other things in the language that would result in undefined behavior when run but are caught consistently, that's why my question is really "is there a compiler which silently emits code that will do undefined things".)

PK

[Daniel Cheng]

Yes. See https://groups.google.com/a/chromium.org/forum/m/#!msg/blink-dev/dFwjUNDGI3k/caPm1A0hCacJ for an example.

Daniel

--

[James Robinson]

This is undefined in the arbitrarily bad and not always compiler detectable way. If you use a const static, *always* define storage. If you don't want to do that, *always* use an enum. static const without storage is playing with fire.

[Jeffrey Yasskin]

The aspect of the ODR ([basic.def.odr]) that Peter wants to risk
violating is "Every program shall contain exactly one definition of
every non-inline function or variable that is odr-used in that
program; no diagnostic required." I'm pretty sure all compiler/linker
systems actually diagnose this one. Not certain, but I don't remember
a missed diagnostic here. It's true, however, that "no diagnostic
required" is a red flag.

Linkers don't diagnose the rule 2 paragraphs later: "There can be more
than one definition of [lots of things] ... provided the definitions
satisfy the following requirements." That's the part the
operator<<(WTF::String) example violates.

However, I think Peter's guideline lays traps for future maintainers
and should be avoided anyway, even though the traps aren't generally
made of undefined behavior. Leaving out the storage for a const static
means someone's going to have to add it when they make some other
innocuous change, which will slow them down and make them get extra
LGTMs. The original author should be conscientious enough to add the
storage instead.

Jeffrey

[Peter Kasting]

On Tue, Aug 11, 2015 at 9:01 PM, Jeffrey Yasskin <jyas...@chromium.org> wrote:

The aspect of the ODR ([basic.def.odr]) that Peter wants to risk
violating is "Every program shall contain exactly one definition of
every non-inline function or variable that is odr-used in that
program; no diagnostic required." I'm pretty sure all compiler/linker
systems actually diagnose this one. Not certain, but I don't remember
a missed diagnostic here. It's true, however, that "no diagnostic
required" is a red flag.

Linkers don't diagnose the rule 2 paragraphs later: "There can be more
than one definition of [lots of things] ... provided the definitions
satisfy the following requirements." That's the part the
operator<<(WTF::String) example violates.

However, I think Peter's guideline lays traps for future maintainers
and should be avoided anyway, even though the traps aren't generally
made of undefined behavior. Leaving out the storage for a const static
means someone's going to have to add it when they make some other
innocuous change, which will slow them down and make them get extra
LGTMs. The original author should be conscientious enough to add the
storage instead.

Thanks.  I think this summarizes things well.  I withdraw the recommendation to use consts where possible, though hopefully I can reinstate it when we can use constexpr.  Until then, at least minimize class-scope enum use in cases where it's possible to just define the constant at function or file scope.  That would take care of a lot of these.

PK 

[Matthew Wolenetz]

Thanks everyone for the detailed discussion. It sounds like the risk of undefined ODR-use-related behavior when using static const missing storage definition is severe enough to warrant clarification of preference for enums in the style guide. WDYT?

[Tima Vaisburd]

From reading the discussion I can conclude that enum is just simpler.

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

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

[Peter Kasting]

On Wed, Aug 12, 2015 at 11:12 AM, Matthew Wolenetz <wole...@chromium.org> wrote:

Thanks everyone for the detailed discussion. It sounds like the risk of undefined ODR-use-related behavior when using static const missing storage definition is severe enough to warrant clarification of preference for enums in the style guide. WDYT?

No, I don't think we should add anything (either way) to the style guide.

The Google style guide doesn't find this important enough to call out, and there's nothing unique about Chromium that makes it any different than Google in this particular regard.

PK 

[Xiaohan Wang]

Wow, I leaned a lot from this thread.

What do we do about existing class level static consts? I think in many ("most" I guess) cases we are not adding a storage.

https://code.google.com/p/chromium/codesearch#search/&q=%5E%5C%20%5C%20static%5C%20const.*=.*;&sq=package:chromium&type=cs

[Peter Kasting]

On Mon, Aug 17, 2015 at 2:18 PM, Xiaohan Wang <xhw...@chromium.org> wrote:

Wow, I leaned a lot from this thread.

What do we do about existing class level static consts? I think in many ("most" I guess) cases we are not adding a storage.

https://code.google.com/p/chromium/codesearch#search/&q=%5E%5C%20%5C%20static%5C%20const.*=.*;&sq=package:chromium&type=cs

It seems like if these are compiling fine today we shouldn't intentionally go back and try to convert them all to enums.  (That's doubly true if we think that someday we might want to convert to constexprs.)

PK 

[Xiaohan Wang (王消寒)]

Understood. But potentially we could have hidden undefined behavior in the existing code which isn't causing compiling errors, right? I guess we are fine since it *seems* fine so far in practice?

[Peter Kasting]

On Mon, Aug 17, 2015 at 2:26 PM, Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Understood. But potentially we could have hidden undefined behavior in the existing code which isn't causing compiling errors, right?

See Jeff Yaskin's message; it seems unlikely that there's undefined behavior that's currently not being diagnosed by any compiler.  The risks of these are more in terms of possibly causing compile failures if future maintainers change things.

PK 

[Xiaohan Wang (王消寒)]

I see. I misunderstood the main reason of preferring enum. Thanks for the clarification!

[Matthew Wolenetz]

I too had missed the import of Jeff Yaskin's message, and probably myself caused  Xiaohan's confusion. My apologies.

[Dana Jansens]

A question of style came up in regards to this thread in a recent code review.

Currently we have code such as

static const int kMyConstant = 5;

We'd be changing that based on guidance to an enum. My first expectation was that it would look like

enum : int { kMyConstant = 5 };

But it was pointed out that technically the chromium styleguide overrides the google one on this point, and would expect the name of the constant to change to MY_CONSTANT.

Personally, I don't think that makes a lot of sense in this case, it would require changing all uses of the constant, and misrepresent it's intentions (I'd expect MY_CONSTANT to be one of a set/bitset of other things). So I would prefer kMyConstant.

My proposal is that enums-as-constants (ie. enums with a single member with an explicit value) be written kMyConstant as they would for non-enum constants. I hope this won't displease folks.

[Vladimir Levin]

On Mon, Nov 2, 2015 at 3:04 PM, Dana Jansens <dan...@chromium.org> wrote:

A question of style came up in regards to this thread in a recent code review.

Currently we have code such as

static const int kMyConstant = 5;

We'd be changing that based on guidance to an enum. My first expectation was that it would look like

enum : int { kMyConstant = 5 };

But it was pointed out that technically the chromium styleguide overrides the google one on this point, and would expect the name of the constant to change to MY_CONSTANT.

Personally, I don't think that makes a lot of sense in this case, it would require changing all uses of the constant, and misrepresent it's intentions (I'd expect MY_CONSTANT to be one of a set/bitset of other things). So I would prefer kMyConstant.

My proposal is that enums-as-constants (ie. enums with a single member with an explicit value) be written kMyConstant as they would for non-enum constants. I hope this won't displease folks.

I would support this in cases where there is exactly one name with a specified value, since the intent of the code is to declare a constant. Would this be a change to the style guide regarding ENUM_NAMING (ie an exception to the rule)?

 

On Mon, Aug 17, 2015 at 2:49 PM, Matthew Wolenetz <wole...@chromium.org> wrote:

I too had missed the import of Jeff Yaskin's message, and probably myself caused  Xiaohan's confusion. My apologies.

On Aug 17, 2015 2:34 PM, "Xiaohan Wang (王消寒)" <xhw...@chromium.org> wrote:

I see. I misunderstood the main reason of preferring enum. Thanks for the clarification!

On Mon, Aug 17, 2015 at 2:29 PM, Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Aug 17, 2015 at 2:26 PM, Xiaohan Wang (王消寒) <xhw...@chromium.org> wrote:

Understood. But potentially we could have hidden undefined behavior in the existing code which isn't causing compiling errors, right?

See Jeff Yaskin's message; it seems unlikely that there's undefined behavior that's currently not being diagnosed by any compiler.  The risks of these are more in terms of possibly causing compile failures if future maintainers change things.

PK 

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

--
--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

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

[Peter Kasting]

On Mon, Nov 2, 2015 at 3:10 PM, Vladimir Levin <vmp...@google.com> wrote:

On Mon, Nov 2, 2015 at 3:04 PM, Dana Jansens <dan...@chromium.org> wrote:

A question of style came up in regards to this thread in a recent code review.

Currently we have code such as

static const int kMyConstant = 5;

We'd be changing that based on guidance to an enum. My first expectation was that it would look like

enum : int { kMyConstant = 5 };

But it was pointed out that technically the chromium styleguide overrides the google one on this point, and would expect the name of the constant to change to MY_CONSTANT.

Personally, I don't think that makes a lot of sense in this case, it would require changing all uses of the constant, and misrepresent it's intentions (I'd expect MY_CONSTANT to be one of a set/bitset of other things). So I would prefer kMyConstant.

My proposal is that enums-as-constants (ie. enums with a single member with an explicit value) be written kMyConstant as they would for non-enum constants. I hope this won't displease folks.

I would support this in cases where there is exactly one name with a specified value, since the intent of the code is to declare a constant. Would this be a change to the style guide regarding ENUM_NAMING (ie an exception to the rule)?

Right.  I encouraged Dana to write this email because I think the proposal here makes sense but the wording of the Chromium style guide needs to be tweaked to allow this flexibility.

PK 

[Bruce Dawson]

The CL in question is:

https://codereview.chromium.org/1428003002

This CL was necessitated by VC++ 2015 Update 1 which now requires a storage declaration in some cases for static const int variables. This serves as a reminder that another reason to prefer enum to static const int for constants is it avoids complicated compatibility discussions.

Renaming these consts would require more churn. I think we shouldn't require renaming in order to change how a constant is declared.

[Greg Thompson]

Seconded.

[Peter Kasting]

On Mon, Nov 2, 2015 at 3:14 PM, Peter Kasting <pkas...@chromium.org> wrote:

I encouraged Dana to write this email because I think the proposal here makes sense but the wording of the Chromium style guide needs to be tweaked to allow this flexibility.

Since sentiment has been positive so far and I don't think this is very controversial, I'm going to go ahead and make the Chromium style guide change.  If people disagree with this feel free to speak up, but I don't really expect any objection.

PK

[Yuri Wiitala]

Hey,

Did this style guide change ever happen?  I wanted to refer to it on a codereview I'm doing right now.  I've checked both https://www.chromium.org/developers/coding-style and https://www.chromium.org/developers/coding-style/cpp-dos-and-donts.

--

[Peter Kasting]

On Mon, Nov 30, 2015 at 4:07 PM, Yuri Wiitala <m...@chromium.org> wrote:

Hey,

Did this style guide change ever happen?  I wanted to refer to it on a codereview I'm doing right now.  I've checked both https://www.chromium.org/developers/coding-style and https://www.chromium.org/developers/coding-style/cpp-dos-and-donts.

Yes, it did happen.  Here's the current text of the relevant paragraph, from your first link:

"Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming.  In enums that are actually enumerations (i.e. have multiple values), continue to use this style for consistency.  Use kConstantNaming when using the "enum hack" to define a single constant, as you would for a const int or the like."

PK 

[Fredrik Hubinette]

Shouldn't it just say "use enums instead of static const" (with a list of pros and cons) somewhere?

--

--
Chromium Developers mailing list: chromi...@chromium.org
View archives, change email options, or unsubscribe:
http://groups.google.com/a/chromium.org/group/chromium-dev

---
You received this message because you are subscribed to the Google Groups "Chromium-dev" group.

[Yuri Wiitala]

Yes, I saw that.  I meant, was there a summary of the pro/con discussion about when to use the enum hack?

In particular, I want to define a constexpr int in a header file included by a bunch of other files.  This is not in a class, but in a namespace.  I don't want the compiler to feel it needs to allocate storage and/or complicate things with export/linker foo across components.

[Dana Jansens]

On Mon, Nov 30, 2015 at 4:15 PM, Yuri Wiitala <m...@chromium.org> wrote:

Yes, I saw that.  I meant, was there a summary of the pro/con discussion about when to use the enum hack?

In particular, I want to define a constexpr int in a header file included by a bunch of other files.  This is not in a class, but in a namespace.  I don't want the compiler to feel it needs to allocate storage and/or complicate things with export/linker foo across components.

You can not use constexpr yet. http://chromium-cpp.appspot.com/#core-blacklist

Always use an enum in headers IMO. Just use a static variable in .cc files.

[Peter Kasting]

On Mon, Nov 30, 2015 at 4:15 PM, Yuri Wiitala <m...@chromium.org> wrote:

Yes, I saw that.  I meant, was there a summary of the pro/con discussion about when to use the enum hack?

No, that is not summarized in the style guide anywhere.

In particular, I want to define a constexpr int in a header file included by a bunch of other files.  This is not in a class, but in a namespace.  I don't want the compiler to feel it needs to allocate storage and/or complicate things with export/linker foo across components.

As Dana said, use an enum for now as constexpr is banned.

Likely, when we can finally approve constexpr, there will be some discussion as to whether it's better than the "enum hack" in any cases.

PK 

[Yuri Wiitala]

Ok.  Thanks for the clarifications, Dana and Peter.

Yep, I knew we can't use constexpr yet, that's why I wanted to use the enum hack in my header file.  :)

[Vladimir Levin]

I'd just like to clarify something (several months later). I was reading http://stackoverflow.com/questions/34445336/constexpr-global-constants-in-a-header-file-and-odr and as far as I understand it constexpr doesn't help this whole issue, does it? That is, you can have a global constexpr variable and take its address which is an odr use. Am I missing something?

[Peter Kasting]

On Wed, Apr 13, 2016 at 3:53 PM, Vladimir Levin <vmp...@google.com> wrote:

I'd just like to clarify something (several months later). I was reading http://stackoverflow.com/questions/34445336/constexpr-global-constants-in-a-header-file-and-odr and as far as I understand it constexpr doesn't help this whole issue, does it? That is, you can have a global constexpr variable and take its address which is an odr use. Am I missing something?

Sadly, I believe constexpr vars have the same potential ODR pitfalls as const vars.  What I don't know is whether

constexpr int kFoo = ...;

allows the compiler to do any additional compile-time evaluations (e.g. in other constexpr expressions), compared to

enum { kFoo = ... };

If it doesn't, then basically the above guidance is unchanged.  If it does, then there's a tradeoff to consider :/

PK