kCamelCaseEnums, revisted

[Daniel Cheng]

Currently, Chromium style explicitly disallows the use of kStyleNaming for enum values. One of the reasons is for compatibility with the Java style, which names constants LIKE_THIS.

However, I'm running into instances where enum value names collide with system macros. https://codereview.chromium.org/1553493002 is a change which requires adding new headers. However, this causes random build breakage because ui/events/keycodes/dom/keycode_converter_data.inc has enum values named KEY_A, KEY_B, etc. and <linux/input.h> also defines macros named KEY_A, KEY_B, etc.

Can we reconsider allowing enum values to be named with kConstantStyle to avoid macro collisions? This is consistent with Google style, not (imo) confusing, and makes header ordering much less fragile. The last is really the largest benefit:  PS2 of the linked patch tried to fix the issue, but it turns out that a header file included by ui/events/ozone/evdev/event_converter_evdev_impl.h also includes <linux/input.h>.

Finally, it'll also make it easier for automated tools that sort includes... and really, who wants to sort their includes manually if they don't have to?

Daniel

[Fady Samuel]

This seems like fundamentally a scoping issue to me. What about switching enums over to enum classes now that we support that C++11 feature? Thoughts?

Fady

--
--
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.
To unsubscribe from this group and stop receiving emails from it, send an email to chromium-dev...@chromium.org.

[Daniel Cheng]

Sadly, that doesn't help because the preprocessor will mangle those names before the compiler ever sees it.

Daniel

[Fady Samuel]

Ohh good point. I forgot about that.

Fady

[Nico Weber]

I think right now is a bad time to talk about this: From what I hear, this is being discussed google-internally (because of enum classes); and the blink style discussions are ongoing too (and not changing Chromium's enum naming matched blink's preferred style iirc).

The constraints are that we want chromium and blink style to match, and that we want to get closer to the official google style guide over time. Since all of these are in flux in this area right now, it seems better to wait a few weeks to see how things settle down.

[Peter Kasting]

We're definitely off the rails if we have to address this sort of thing via careful #include ordering to sidestep name conflicts -- that's a recipe for unexpected build breakage down the road.

However, it's not obvious that "use kConstant names instead of MACRO_ONES" is the only or correct solution here.   If you're already proposing renaming the enum values, it would presumably be just as possible to find a more unique MACRO_STYLE name.  Or wrap the enum in a namespace or something.

Really, what I'm concerned with is that a rule "always use SHOUTY_CASE" and a rule "always use kCamelCase" are both clear and easy to follow, but a rule like "do whatever, but Chromium is 99.9% one way" is not so palatable.  It would be better IMO to switch completely to "use camel case for all enum values, everywhere, from now on" than to just allow it in a few cases or at author discretion.

If as Nico says this is being discussed internally as well, and thus the Google style guide itself may change in the future, then perhaps indeed waiting is best, as we don't want to change to match something that gets pulled out from under us.  (In the meantime, can one of my suggestions above be used to fix this without fragile #include ordering tricks?)

PK

[Daniel Cheng]

Yeah, I didn't realize there was an internal discussion ongoing. I agree it makes sense to see how things settle down before thinking about changes to this area.

Namespaces won't help, since macros don't respect scoping. A rename would certainly solve the immediate problem, but it's not clear what a better name would be: the enum is DomCode, and KEY_A, KEY_B, etc are very reasonable names for enumerators.

Daniel

[Peter Kasting]

On Mon, Dec 28, 2015 at 6:49 PM, Daniel Cheng <dch...@chromium.org> wrote:

Namespaces won't help, since macros don't respect scoping.

Can we avoid using macros to define all this stuff?

 

A rename would certainly solve the immediate problem, but it's not clear what a better name would be: the enum is DomCode, and KEY_A, KEY_B, etc are very reasonable names for enumerators.

DOM_KEY_A?  KEY_CODE_A?

PK 

[Daniel Cheng]

On Mon, Dec 28, 2015 at 6:50 PM Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Dec 28, 2015 at 6:49 PM, Daniel Cheng <dch...@chromium.org> wrote:

Namespaces won't help, since macros don't respect scoping.

Can we avoid using macros to define all this stuff?

It's not a macro defined by Chromium code that's the problem: it's (many) macros from a system header (in this case, <linux/input.h>) that conflict with names that Chromium code also uses.

 

 

A rename would certainly solve the immediate problem, but it's not clear what a better name would be: the enum is DomCode, and KEY_A, KEY_B, etc are very reasonable names for enumerators.

DOM_KEY_A?  KEY_CODE_A?

That seems arguably worse to me, but that's just bikeshedding =)

 

PK 

[Peter Kasting]

On Mon, Dec 28, 2015 at 7:03 PM, Daniel Cheng <dch...@chromium.org> wrote:

On Mon, Dec 28, 2015 at 6:50 PM Peter Kasting <pkas...@chromium.org> wrote:

On Mon, Dec 28, 2015 at 6:49 PM, Daniel Cheng <dch...@chromium.org> wrote:

Namespaces won't help, since macros don't respect scoping.

Can we avoid using macros to define all this stuff?

It's not a macro defined by Chromium code that's the problem: it's (many) macros from a system header (in this case, <linux/input.h>) that conflict with names that Chromium code also uses.

Thanks, I misconceived the problem in my head.  I understand now, even though I should have after you shot down the enum class suggestion :)

A rename would certainly solve the immediate problem, but it's not clear what a better name would be: the enum is DomCode, and KEY_A, KEY_B, etc are very reasonable names for enumerators.

DOM_KEY_A?  KEY_CODE_A?

That seems arguably worse to me, but that's just bikeshedding =)

I don't necessarily disagree, but it does seem like a solution that could be implemented in the short term, and at least the damage is more in the "arguable" realm than the "holy crap, that's hideous" realm.

PK 

[Daniel]

+kpschoedel@

Kevin can shed more light for this specific use case (dom codes), once he's back from vacation.

[Vladimir Levin]

(resending to chromium-dev, since my reply bounced back)

On Tue, Dec 29, 2015 at 12:38 PM, Vladimir Levin <vmp...@chromium.org> wrote:

According to https://google.github.io/styleguide/cppguide.html#Enumerator_Names, SHOUTY_CASE was recommended until Jan 2009 and because of exactly the problem dcheng@ brought up here, it was changed to prefer kCamelCase (ie new code should prefer kCamelCase). 

I assume that the naming change that is being discussed internally mostly deals with enum classes, since I think it would be kind of hazardous to change the names for regular enums (ie enum Foo { bar } will collide with anything named bar, whereas enum class Foo { bar } doesn't have that issue). (I might be wrong here, since I could only find partial discussion threads)

Waiting for internal discussion to come to a consensus sgtm. But maybe considering loosening the rules around regular enum naming is also worth doing. 

Just my 2c.

--

[Kevin Schoedel]

Oh, we're already in the "holy crap, that's hideous" realm. The plan at the time was to rename the DomCode::KEY_X values to DomCom::X, but I evidently forgot to do that. (The names KEY_X come from mapping the CamelCase forms in the W3C spec, though there are already some exceptions. “Key…” is used there only for Latin letters.) Assuming it introduces no other conflicts, I will do the renaming now and remove the workarounds.