Recently it came across to us that some of the new code does not follow an intended code style. Partially the style itself is at fault, as it apparently used ambiguous wording, without an example to make it clear. We’ve fixed the style guide now to make it unambiguous. But we are left in a situation where we have inconsistency within the code, which we need to resolve.
There are two main contenders when it comes to the style of enumerator values:
ALL_CAPITAL_SNAKE_STYLE (this is the one which is supposed to be official by the style guide)
TitleCase (or, PascalCase, sometimes referred to as CamelCase, but not to be confused with camelCase).
We also have a legacy of C-style enums (as opposite to the enum class in C+±11), which adds some extra permutations for finding the way forward.
Before digging into the permutations, the common note to be mentioned: change in style is always going to be disruptive. If we change style, we’d typically expect it to be enforced in all the key parts of Blender (including some of the intern/, such as Cycles), and we do not want changes in style to be in the realms of only applicable to the new code as doing so has its own downsides. I’d like you to keep that in mind.
So when it comes down to possibilities:
Stick to SNAKE_STYLE for both enum and enum class
This is how the style was intended to be, but it’ll mean the enum classes would need to be changes in all the new areas (and people might have the most emotional attachment and motivation for sticking to it, as the new code is typically from the most active developers).
Switch to the TitleCase for both enum and enum class
This is better aligned with the style preferred in the current active developers in new C++ code in Blender, but is the most disruptive, and also makes those TitleCase being in the global namespace
Stick to the SNAKE_STYLE for enum but use TitleCase for enum class. Kindof middle ground, which does not require big disruptive changes, and keeps active C++ developers happy. The downside is what if we ever switch enum to enum class, and what to do with [anonymous] enums inside of a class (sometimes is does need to happen).
Maybe some other permutation which is worth mentioning which I forgot?
What I’d like to do is to bring this up to the discussion at the coming bf-admin meeting, and hopefully come to a resolution. And what I’d like to hear from you before that meeting:
What is the style you’d like to become the official style (for both enum and enum class).
If it’s TitleCase what would you propose for dealing with abbreviations (IOR, BSDF, CCGDM, etc. FooIORBar vs FooIorBar).
What are your thoughts about alternatives, on a scale "I'd tolerate it very much" to "I immediately fork Blender and apply the only objectively correct style".
I don’t think I’d want to give own thoughts on that just yet, to not cause any bias or anything like that. I’d say that as long as the team is happy and code is consistent with the style guide is all it really matters.
You’re mixing up camelCase and PascalCase also i think i heard hans say yesterday the new code actually used TitleCase best to get these definitions straight before we all get really really confused.
Edit: seems like i’m the one confused, apparently camel case seemingly allows the first letter to be either, heh… who knew…
First of all, i think it is important to describe the logic of the rule. So i will try/
No one like CAPS text. And there is should be the reason to use this upper case (all upper) for the names. Possibility to leak. Things that is written by CAPs right now in blender code base is the macro and C-style enums (enum {}) in C headers that is unstoppable by the C++ concepts like namespaces.
So, the behavior for us should be so to explicitly mark C-style enums as such and keep any other things separately.
If thing is in any kind of namespace, there is no reason for its separation by the name prefix or different register.
It is not in style guide but BKE_node_size is totally meaningless in C++, and after C++ convertion now there is blender::bke::node_size. In other case i do not know why we even have bke and blender namespaces.
And the same story with enums. If enum is in its namespace by default (enum class), because this is C++ file, it’s clearly reason to do not use name as way to mark the name.
And i’d would just prohibit unnamed enums…
This probably should have been cleared up before if C++ conversion. There is C++ features but C code style.
Not that it matters much, as because of time scarcity I’m (currently) not a very active developer, but just to add an opinion:
Even though “No one like CAPS text” as Iliya says, it is one of the ‘traditional’ ways to name enum values. I’m probably an old fart but when I see SNAKE_CASE_NAMES I immediately think ‘enum or #define’.
This is true across a lot of (older) codebases and so it makes it imo easier to understand the code as a newcomer.
camelCase is used for variable names and TitleCase is used for function names in a lot of codebases. And while on the one hand blender doesn’t really need to regard other projects’ styleguides, it does add some context switching cost for especially outside programmers who might be active in multiple codebases (like myself, of course )
Feel free to disregard my opinion, because as said I’m not too active these days (though I hope to be again, in the future).
edit:
Oops, sorry Iliya, I meant to reply the topic, not you specifically.
To maybe make it a bit more relevant: warning about namespace leaking is of course one possible function of the SNAKE_CASE (“big bold letters: beware”) but the conventions ‘this is probably an enum’ is a useful one as well I think.
TL;DR: I’d rather stick to current style, but won’t fight to death for it either. The only thing I’d rather really not see happening is the mixed style of SNAKE_CASE for C-style enum, and TitleCase for Cpp-style of enum class. IMHO that would only add to the confusion.
I do not have strong preference or opinion in general, though I’m also an old fart who’s used to the SNAKE_CASE style and finds it nice and easy to identify ‘constants’ in code. But could also live with the TitleCase situation.
The main reason I’d rather stick with existing style is that I see zero benefit to change it, and it will be yet another costly ‘useless’ noisy change in our codebase. These types of change always require several days of dev-work, are a pain for all PRs and dev branches, are a pain for bisecting issues, and so on. I believe this time can be invested in more useful work.
After getting replies i see a small discrepancy of ideas:
Using enums as way to define constans or using SNAKE_CASE as way to say that this is the constant is just wrong way to go in new code (C++). Should we use the same logic for const or constexpr variables? Enums should be the way to collect constants in to the name space with the same space-extension and type. Enum is not the way to hack compile and create compile time number. This might was such in C, but we can do constexpr int a = b * 4 + func(b); in C++. And this should not be constexpr A = ... just because this is semantically the same as enum { A = ... };.
I think many people here understand this. Having debates of what enum and enum classes are good for and do not really move us forward. Please answer information requested from the original post.
I would strongly prefer to use PascalCase for enum class, but stick with with ALL_CAPS for enum and #define. The reasoning is that enum class forces you to write the name prefix, and AttrDomain::Point is much more pleasant on the eyes than ATTR_DOMAIN_POINT.
That isn’t a disruptive change, since existing code could stay the same, even if it follows the guidelines exactly. For new code generally enum class should be used over enum, and the change could also be applied as work is done in certain areas and enum is changed to enum class.
I would handle abbreviations like IOR in a case by case basis or just not include it in the style guide, it seems things get a bit too specific at that point, though that doesn’t matter much to me.
I just tried to move arguments from area here is code base with such style or the same style is used in other language to area of this style used for that reason… will stop\
My personal preference agrees with Hans’s, but I’ll do whatever the rule turns out to be.
Just as a point of interest, both the Core Guidelines and the Google C++ Style Guide
advise against using all caps for enumeration names, for the reason that they advise using all caps for macros and want to keep all caps exclusively for the macros case.
This would also be my preference after some thought.
All caps is an eyesore from the past. It had its place, but with more modern language features (enum class being one of them), I don’t see a need for them anymore.
I rather keep the all caps for C-style code. The eyesore is justified there, because it potentially indicates places that don’t use the modern features that make all caps unnecessary. We all agree macros are bad, so it’s good that they are easy to spot. The same could be said about enums that would be safer if converted to enum classes.
This topic has been discussed at today’s admin meeting (notes).
The main take away:
Overall it is fine to use TitleCase for enum class, but stick to SNAKE_STYLE for enum and macro.
We do need to have a policy for abbreviations.
The proposal could be to capitalize the first letter of acronyms. Possibly follow Camel case: defined from Google.
There is a hard feeling about ID, because it is what it is, and is not a short for “identifier”. Don’t think there is anything wrong with saying that ID is ID. Unless someone is willing to be verbose, and spell it as DataBlock in the enum values
If this is off topic and should be discussed in it’s own topic, I’ll create a new one, but I was wondering if this proposal should be extended to refactoring similar constant values, i.e. the case of
#define SOME_CONSTANT_VALUE -1
being turned into
static constexpr int SOME_CONSTANT_VALUE = -1;
or
static constexpr int some_constant_value = -1;
This came up in a discussion in a recent PR, and I’ve seen both formats used across the codebase.
That is a good question. There definitely seems to be synergy with this topic.
The least controversial and can-be-applied-right-away-no-quesitons-asks would be the static constexpr int SOME_CONSTANT_VALUE = -1;.
If we want to have some deeper discussion, I think we’d better start a new thread, and maybe just acknoledge that similar discussion happen in an adjacent thread. Will give it a better place for people to raise their strong opinions.
If you feel motivated to help with this topic, feel free to start one, and poke blender-coders etc.
)
