Policy for style guide: document enum suffix use for C/C++ #99080

New Issue

Campbell Barton · 2022-06-22T07:22:09+02:00

Campbell Barton commented

2022-06-22 07:22:09 +02:00

Currently most typedef'd enums use an e prefix, although this isn't mentioned in the code style guide.

As this isn't noted as a convention it's not clear if/when they should be used if it's reasonable to request them in code-review for example.

I'd propose to use a limited set of suffixes for enum types, which should be limited to enum types.

Based on existing suffix use, I'd suggest the following:

Type
Mode
Flag
Mask

This implies the following changes:

Remove e prefix from existing enum names.
Existing types that aren't enums be renamed (e.g. AbstractPaintMode, MFDataType, IndexMask, ObCustomData_ForEditMode, CPPType).
Existing enums that don't use these extensions should have them added (e.g. BlurAxis, ThreadingModel, eScrollZone).

NOTE: this is a counter proposal to #98553 (Policy for style guide: document use of 'e' prefixed enums in C/C++).

Currently most typedef'd enums use an e prefix, although this isn't mentioned in the code style guide. As this isn't noted as a convention it's not clear if/when they should be used if it's reasonable to request them in code-review for example. I'd propose to use a limited set of suffixes for enum types, which should be limited to enum types. Based on existing suffix use, I'd suggest the following: - `Type` - `Mode` - `Flag` - `Mask` ---- This implies the following changes: - Remove `e` prefix from existing enum names. - Existing types that aren't enums be renamed (e.g. `AbstractPaintMode`, `MFDataType`, `IndexMask`, `ObCustomData_ForEditMode`, `CPPType`). - Existing enums that don't use these extensions should have them added (e.g. `BlurAxis`, `ThreadingModel`, `eScrollZone`). ---- NOTE: this is a counter proposal to #98553 (Policy for style guide: document use of 'e' prefixed enums in C/C++).

Campbell Barton commented

2022-06-22 07:22:09 +02:00

Added subscriber: @ideasman42

Campbell Barton changed title from ~~Policy for style guide: document use common enum suffix in C/C++~~ to Policy for style guide: document enum suffix use for C/C++

2022-06-22 07:31:02 +02:00

Jeroen Bakker commented

2022-06-22 08:40:18 +02:00

Added subscriber: @Jeroen-Bakker

Jeroen Bakker commented

2022-06-22 08:40:18 +02:00

Not sure about to reserve Mask and Mode suffix for enums only. Mask is something that already exists in the user domain.
I would allow Mode when we refer to edit/object/paint mode as it refers to something concrete and also present in the user domain.

NOTE: AbstractPaintMode will be phased out after paint mode has been introduced and the current texture paint mode will be phased out.

Not sure about to reserve Mask and Mode suffix for enums only. Mask is something that already exists in the user domain. I would allow Mode when we refer to edit/object/paint mode as it refers to something concrete and also present in the user domain. NOTE: AbstractPaintMode will be phased out after paint mode has been introduced and the current texture paint mode will be phased out.

Sergey Sharybin commented

2022-06-22 10:43:22 +02:00

Added subscriber: @Sergey

Sergey Sharybin commented

2022-06-22 10:43:22 +02:00

Generally fine.

Although, would be nice to clarify what is Flag and what is Mask. When to sue what. Or, stick to one even :)
To solve naming ambiguity we can call it Bitmask?

Generally fine. Although, would be nice to clarify what is Flag and what is Mask. When to sue what. Or, stick to one even :) To solve naming ambiguity we can call it Bitmask?

Bastien Montagne commented

2022-06-27 11:13:00 +02:00

Added subscriber: @mont29

Bastien Montagne commented

2022-06-27 11:13:00 +02:00

Generally fine with the proposal.

Am not sure that Bitmask vs. Flag would be less confusing, since most of our existing Flag ones are bitflags I think? But agree with @Jeroen-Bakker, not sure why we need the Mask one at all?

Would add Tag though, in ID/Core area we use the convention that Flag is for persistent data (saved/read to/from disk), while Tag is for runtime data (therefore cleared on file load).

Generally fine with the proposal. Am not sure that `Bitmask` vs. `Flag` would be less confusing, since most of our existing `Flag` ones are bitflags I think? But agree with @Jeroen-Bakker, not sure why we need the `Mask` one at all? Would add `Tag` though, in ID/Core area we use the convention that `Flag` is for persistent data (saved/read to/from disk), while `Tag` is for runtime data (therefore cleared on file load).

Campbell Barton commented

2022-06-27 12:14:51 +02:00

@mont29 Removing Mask suffix may be OK, although I'm not sure of a good alternative in some cases.

Checking Blender's code, some uses could be replaced with flag

Example usage in Blender:

eEventType_Mask (not directly converted from a type so could arguably be renamed to EventType_Flag).
eAttrDomainMask
eCustomDataMask

These are examples of a case where removing the suffix "Mask" doesn't fit so well with other naming conventions.

When the flag is created from bit shifting a type, e.g. mask = (1 << type), so it's possible to store a singe flag that represent multiple types. eAttrDomainMask is an example of this.

Calling it AttrDomainFlag would be misleading as it would read as if it were various options for the attribute-domain.

Since this use case isn't all that common, we could consider something more verbose although nothing jumps to mind as being an obvious good choice as most combinations of terms end up being as overly verbose.

Nothing against including Tag although it's usage seems isolated to a few areas.

@mont29 Removing `Mask` suffix may be OK, although I'm not sure of a good alternative in some cases. Checking Blender's code, some uses could be replaced with flag Example usage in Blender: - `eEventType_Mask` (not directly converted from a type so could arguably be renamed to `EventType_Flag`). - `eAttrDomainMask` - `eCustomDataMask` These are examples of a case where removing the suffix "Mask" doesn't fit so well with other naming conventions. When the flag is created from bit shifting a `type`, e.g. `mask = (1 << type)`, so it's possible to store a singe flag that represent multiple types. `eAttrDomainMask` is an example of this. Calling it `AttrDomainFlag` would be misleading as it would read as if it were various options for the attribute-domain. Since this use case isn't all that common, we could consider something more verbose although nothing jumps to mind as being an obvious good choice as most combinations of terms end up being as overly verbose. ---- Nothing against including `Tag` although it's usage seems isolated to a few areas.

Hans Goudey commented

2022-06-27 18:19:34 +02:00

Added subscriber: @HooglyBoogly

Hans Goudey commented

2022-06-27 18:19:34 +02:00

Personally I don't see the benefit in such strict naming requirements for these cases. For example, renaming IndexMask or CPPType hurts more than it helps. Those are good, semantically correct names that are easy to say and read. Forcing these strict requirements is just going to make things more awkward.

I also don't see the benefit of making such a distinction between "enum types" and "other types". It's nice to know when to pass types by value, but their "enum-ness" isn't a good way to measure that, since plenty of other types should be passed by value. It can also make sense to pack other data with an "enum type" in a class. These rules wouldn't allow for such gray area, and might end up making naming worse.

Personally I don't see the benefit in such strict naming requirements for these cases. For example, renaming `IndexMask` or `CPPType` hurts more than it helps. Those are good, semantically correct names that are easy to say and read. Forcing these strict requirements is just going to make things more awkward. I also don't see the benefit of making such a distinction between "enum types" and "other types". It's nice to know when to pass types by value, but their "enum-ness" isn't a good way to measure that, since plenty of other types should be passed by value. It can also make sense to pack other data with an "enum type" in a class. These rules wouldn't allow for such gray area, and might end up making naming worse.

Brecht Van Lommel commented

2022-06-27 19:01:06 +02:00

Added subscriber: @brecht

Brecht Van Lommel commented

2022-06-27 19:01:06 +02:00

I think we could recommend using Type, Mode, Flag and Mask for most enum names, but not make it hard requirement, and not require other types to not use them.

In general, I don't think we should aim to make the name unambiguously explain the type, be it through Hungarian notation or some other convention. I think it helps to aim for consistent naming of data structures with a similar purpose, but I don't think it matters as much if the actual type is an enum, enum class, typedef, class or struct.

I think we could recommend using `Type`, `Mode`, `Flag` and `Mask` for most enum names, but not make it hard requirement, and not require other types to not use them. In general, I don't think we should aim to make the name unambiguously explain the type, be it through Hungarian notation or some other convention. I think it helps to aim for consistent naming of data structures with a similar purpose, but I don't think it matters as much if the actual type is an `enum`, `enum class`, `typedef`, `class` or `struct`.

Campbell Barton commented

2022-07-06 12:46:34 +02:00

While I can see that not being strict on this has some benefits (as there are a handful of times when some other names make some sense), most of our types already follow the convention, so I'm not convinced it's a overall benefit, mainly because enums are used differently to most other types.

This is especially the case when jumping into a new area of Blender, things are may be named strangely - it's not clear if it's poor naming that should be changed or something the original author would prefer to keep as-is. Examples of enum names already in use are: matAnim, format, eAlpha, MyEnum, TBPFLAG (some are in namespaces so it's not as bad, nevertheless, I don't think they're great). Not having a convention most likely means we would continue as is (while removing the e prefix).

If we could follow @brecht's suggestion as a but make this a stronger recommendation, then I think things would be more manageable.
A strong recommendation would mean there would need to be a good reason not to use one of the suggested suffixes.

While I can see that not being strict on this has some benefits (as there are a handful of times when some other names make some sense), most of our types already follow the convention, so I'm not convinced it's a overall benefit, mainly because enums are used differently to most other types. This is especially the case when jumping into a new area of Blender, things are may be named strangely - it's not clear if it's poor naming that should be changed or something the original author would prefer to keep as-is. Examples of enum names already in use are: `matAnim`, `format`, `eAlpha`, `MyEnum`, `TBPFLAG` (some are in namespaces so it's not as bad, nevertheless, I don't think they're great). Not having a convention most likely means we would continue as is (while removing the `e` prefix). If we could follow @brecht's suggestion as a but make this a stronger recommendation, then I think things would be more manageable. *A strong recommendation would mean there would need to be a good reason not to use one of the suggested suffixes.*

Sybren A. Stüvel commented

2022-07-12 12:41:09 +02:00

Added subscriber: @dr.sybren

Sybren A. Stüvel commented

2022-07-12 12:41:09 +02:00

I agree with @HooglyBoogly.

For some proposed "these need to be changed" names, I don't immediately see what would be improved. What would BlurAxis or ThreadingModel be changed to? An axis is just an axis, and to me BlurAxis is fine; a BlurAxisType or BlurAxisMode to me introduces the concept of "axis type" or "axis mode", which don't exist. These are also not "flags" or "masks".

I agree with @HooglyBoogly. For some proposed "these need to be changed" names, I don't immediately see what would be improved. What would `BlurAxis` or `ThreadingModel` be changed to? An axis is just an axis, and to me `BlurAxis` is fine; a `BlurAxisType` or `BlurAxisMode` to me introduces the concept of "axis type" or "axis mode", which don't exist. These are also not "flags" or "masks".

Campbell Barton commented

2022-07-13 10:33:03 +02:00

In this case we could consider adding Axis and Model to acceptable suffixes. Are there many other suffixes we're likely to use?

In this case we could consider adding `Axis` and `Model` to acceptable suffixes. Are there many other suffixes we're likely to use?

Sergey Sharybin commented

2022-07-13 12:32:31 +02:00

I do not think this is a proper way of defining style guides. The goal should be to make things to be more clear, and restricting naming to only allow specific set of suffixes is not going to help much. I also do not want to end up in a situation when a perfectly clear naming gets replaced with one of those cleanup commits just based on a fact that style did not foresee some naming suffix.

I do not think we should mix making existing naming more clear with making a strict rule out of it. In the official style guide we can state that naming should clearly indicate intent of the type name (although, that's kinda bit obvious) and suggest list of possible suffixes for common types (i.e. if some enum is a set of flags suggest using Flag suffix). Additionally, state that no enum-specific prefix (such as e is to be used).

As for your question: I think it is malformed. I can not think of a suffixes which we might want to use, but there are enough examples where this concept of using pre-defined suffix does not apply:

Ownership
Distortion
Source, RNAPointerSource, eUpdateSource
Status
Channel
PredictDirection
ResectionMethod
Step
Direction
eCyclicCheckVisibleState
EvaluationStage
NodeClass
OperationCode
...

I do not think this is a proper way of defining style guides. The goal should be to make things to be more clear, and restricting naming to only allow specific set of suffixes is not going to help much. I also do not want to end up in a situation when a perfectly clear naming gets replaced with one of those cleanup commits just based on a fact that style did not foresee some naming suffix. I do not think we should mix making existing naming more clear with making a strict rule out of it. In the official style guide we can state that naming should clearly indicate intent of the type name (although, that's kinda bit obvious) and suggest list of possible suffixes for common types (i.e. if some enum is a set of flags suggest using `Flag` suffix). Additionally, state that no enum-specific prefix (such as `e` is to be used). As for your question: I think it is malformed. I can not think of a suffixes which we might want to use, but there are enough examples where this concept of using pre-defined suffix does not apply: * `Ownership` * `Distortion` * `Source`, `RNAPointerSource`, `eUpdateSource` * `Status` * `Channel` * `PredictDirection` * `ResectionMethod` * `Step` * `Direction` * `eCyclicCheckVisibleState` * `EvaluationStage` * `NodeClass` * `OperationCode` * ...

Campbell Barton commented

2022-07-21 04:19:36 +02:00

While this could be a guide-line, even in these examples I find the names they aren't very clear, something even a guide-line would advise against.

State is frequently used as a suffix for structs that stores the state (RNGState, TaskParallelIteratorState, MaskViewLockState, IMMDrawPixelsTexState).
Source is ambiguous too, is the source data being passed in or the kind of source? Currently there are structs containing source data (ParticleDrawSource, GPUSource, tRKS_DSource) so using SourceType suffix seems reasonable in that case.

The situation I find reasonable to be less strict is when names are clearly intended to be interpreted as a number/ordered-value, e.g. Level, Axis, Direction, Stage, I'm still not convinced there are so many of these that a list that contains them all would be impractical but take the point that it's not necessarily worth attempting to restrict ourselves to a comprehensive list.

While this could be a guide-line, even in these examples I find the names they aren't very clear, something even a guide-line would advise against. - `State` is frequently used as a suffix for structs that stores the state (`RNGState`, `TaskParallelIteratorState`, `MaskViewLockState`, `IMMDrawPixelsTexState`). - `Source` is ambiguous too, is the source data being passed in or the kind of source? Currently there are structs containing source data (`ParticleDrawSource`, `GPUSource`, `tRKS_DSource`) so using `SourceType` suffix seems reasonable in that case. The situation I find reasonable to be less strict is when names are clearly intended to be interpreted as a number/ordered-value, e.g. `Level`, `Axis`, `Direction`, `Stage`, I'm still not convinced there are so many of these that a list that contains them all would be impractical but take the point that it's not *necessarily* worth attempting to restrict ourselves to a comprehensive list.

Sergey Sharybin commented

2022-07-21 09:46:50 +02:00

I see your point about the State, but in some state-machiny type of algorithm State can also be a "number" (or, enum class for that matter).

SourceType this is also ambiguous since Type is also used by things like DrawEngineType, BPyGizmoWithTargetType, RenderEngineType, wmGizmoPropertyType, bpy_lib_Type. Renaming all of those to something else will hurt more than it would help.

I do not want to be spending time discussing specifics of name choices in specific areas. Naming can be improved in many areas, and in many areas using enum class will help even more. But all that will not change my opinion about the current state of the proposal.

So at is currently: no, I am not supporting it. I think you have enough feedback from me, Brecht, Hans to modify it to something what will make all us aligned and support it.

I see your point about the `State`, but in some state-machiny type of algorithm `State` can also be a "number" (or, `enum class` for that matter). `SourceType` this is also ambiguous since `Type` is also used by things like `DrawEngineType`, `BPyGizmoWithTargetType`, `RenderEngineType`, `wmGizmoPropertyType`, `bpy_lib_Type`. Renaming all of those to something else will hurt more than it would help. I do not want to be spending time discussing specifics of name choices in specific areas. Naming can be improved in many areas, and in many areas using `enum class` will help even more. But all that will not change my opinion about the current state of the proposal. So at is currently: no, I am not supporting it. I think you have enough feedback from me, Brecht, Hans to modify it to something what will make all us aligned and support it.

Sybren A. Stüvel commented

2022-07-21 11:43:48 +02:00

I think that, if you want to improve clarity, it'll be much more important to have clear types for "the enum values are bitflags, and this value combines them" instead of just passing int and not documenting what's in there. Or to stop using int for the enum values themselves. Or to get enum support in DNA structs. To me such changes would be much more valuable than some sanctioned list of enum-name-suffixes.

I think that, if you want to improve clarity, it'll be much more important to have clear types for "*the enum values are bitflags, and this value combines them*" instead of just passing `int` and not documenting what's in there. Or to stop using `int` for the enum values themselves. Or to get enum support in DNA structs. To me such changes would be much more valuable than some sanctioned list of enum-name-suffixes.

Campbell Barton commented

2022-07-22 02:01:10 +02:00

@Sergey re-reading the replies so far there doesn't seem to be all that much consensus, some devs are OK with guide-line, some prefer not to have any constraints besides removing e prefix.

@dr.sybren I don't see the need to compare this project with others, the are of course other useful things to spend time on, initially I wrote #98553 with the assumption that putting in writing what was already being done (for the most part) & it would be straightforward. As it happens the old/existing conventions aren't so popular and this ended up being more controversial than I anticipated. Of course some tasks just aren't worth us spending much time on, so if developers think this they can say so too.

Personally I find the idea of having a guideline reasonable, something like what Brecht suggested:

In #99080#1381165, @brecht wrote:
I think we could recommend using Type, Mode, Flag and Mask for most enum names, but not make it hard requirement, and not require other types to not use them.

In general, I don't think we should aim to make the name unambiguously explain the type, be it through Hungarian notation or some other convention. I think it helps to aim for consistent naming of data structures with a similar purpose, but I don't think it matters as much if the actual type is an enum, enum class, typedef, class or struct.

If others are OK with making something along these lines into a guide-line, I'll update the proposal. Otherwise there doesn't seem like enough consensus in the replies here to write an updated proposal - that doesn't risk generating further disagreement/discussion.

@Sergey re-reading the replies so far there doesn't seem to be all that much consensus, some devs are OK with guide-line, some prefer not to have any constraints besides removing `e` prefix. @dr.sybren I don't see the need to compare this project with others, the are of course other useful things to spend time on, initially I wrote #98553 with the assumption that putting in writing what was already being done (for the most part) & it would be straightforward. As it happens the old/existing conventions aren't so popular and this ended up being more controversial than I anticipated. Of course some tasks just aren't worth us spending much time on, so if developers think this they can say so too. ----- Personally I find the idea of having a guideline reasonable, something like what Brecht suggested: > In #99080#1381165, @brecht wrote: > I think we could recommend using `Type`, `Mode`, `Flag` and `Mask` for most enum names, but not make it hard requirement, and not require other types to not use them. > > In general, I don't think we should aim to make the name unambiguously explain the type, be it through Hungarian notation or some other convention. I think it helps to aim for consistent naming of data structures with a similar purpose, but I don't think it matters as much if the actual type is an `enum`, `enum class`, `typedef`, `class` or `struct`. If others are OK with making something along these lines into a guide-line, I'll update the proposal. Otherwise there doesn't seem like enough consensus in the replies here to write an updated proposal - that doesn't risk generating further disagreement/discussion.

Bastien Montagne commented

2022-07-22 09:46:18 +02:00

Am also fine with Brecht proposal.

Sergey Sharybin commented

2022-07-22 11:30:23 +02:00

@ideasman42 Brecht suggestion is, at a very least, very close to the consensus.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Policy for style guide: document enum suffix use for C/C++ #99080