[Development] Naming convention for (scoped) enums

[Tor Arne Vestbø]

> On 15 Aug 2018, at 09:32, Alex Blasche <alexander.blasche at qt.io> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: Tor Arne Vestbø
>> 1. Scoped enums (enum class) for the sake of avoiding name clashes is useful for
>> global enums, but when the enum lives inside a class, the chance that we’ll see a
>> naming clash is minor, and using scoped enums in that case arguably has
>> negative effects on code writability/readability. As a result, we should update
>> the policy to not mandate scoped enums when the enum lives inside a class.
> 
> I don't think we have ever not permitted exceptions to official policy. Therefore, take it for granted that the policy can be ignored such as in the case presented by Allan. Having said that the default should be the use of scoped enums even inside of classes.

Fair enough, but having the policy means following that by default, resulting in patches defaulting to the policy because it’s a policy, not taking each individual case into consideration. Since I’m arguing that the default of preferring scoped enums inside classes will in most cases result in less writable/readable code, I don’t want us to default to something that produces worse results than defaulting to the opposite. 

> I don't consider the longer names detrimental for writability and usability. Writability is easily solved with code completion and readability is actually better because the type adds additional context info. To cite an example from Shawn's patch:
> 
> QQuickPointerDevice::Finger
> QQuickPointerDevice::Mouse
> 
> is far less descriptive than (and not compliant to current naming convention)
> 
> QQuickPointerDevice::PointerType::Finger
> QQuickPointerDevice::DeviceType::Mouse. 
> 

When you write these enums in isolation, yes.

But that’s not how the enums are used in actual code:

  if (eventPoint->state() != QQuickEventPoint::Released && wantsEventPoint(eventPoint))

  if (dev->type() == QQuickPointerDevice::TouchScreen)

vs 

  if (eventPoint->state() != QQuickEventPoint::State::Released && wantsEventPoint(eventPoint))

  if (dev->type() == QQuickPointerDevice::DeviceType::TouchScreen)

The context is already there in most cases, which makes forcing the name of the enum into the code redundant.

And if the context is not there, you can still write QQuickPointerDevice::DeviceType::TouchScreen to be more explicit _when_ needed, even without scoped enums.

Or just DeviceType::TouchScreen, if you are inside the class. Or even just TouchScreen if the context is obvious from the rest of the call.

Using scoped enums removes this flexibility.

> The uninitiated reader of the code cannot see the additional type context when reading code and therefore someone might interpret Finger as a device type too.

As argued above, the uninitiated reader _can_.

> 
>> 2. Scoped enums for the sake of type safety is a valid use case, but can be solved
>> by promoting a warning to an error, and as such shouldn’t block us moving
>> forward with #1.
> 
> I don't see how a policy of elevating warnings to errors is better that using a language inbuilt error due to strong type safety.

Of course it’s not better, if you focus only on that. The use of a warning-as-error instead of a language inbuilt is a workable practical compromise because the language inbuilt comes with other downside, as discussed above.

Tor Arne