RFC: Style guide for type casts in C++ code

Technical Feedback
1

Here is a proposal for the Blender Style guide to cover type casting topic in C++ code.

Motivation
The style of casting is not consistent across different files even within the same module. Sometimes the cast is not const or type correct, sometimes is overly verbose. Having a single guideline of casts should help the code quality, readability, and onboarding.

The core principles
There are few things which are important when it comes to type casting. This is what the proposal follows:

  • Readability of simple cases.
  • Const-correctness.
  • Type-safety.

Proposal
This is rather short proposal:

The functional-style arithmetic type is chosen over C-style due to the following reasons:

  • Personal experience working with various templated numerical libraries, where it is sometimes required to construct special arithmetic-like type from a templated functor. In those cases T(something) shows the intent more clearly.
  • Consistent with the way how cast is done in Python.
  • Looks the same as static/reinterpret/const cast.

Some possible extensions for the proposal to cover typical cases in the code:

  • Use static_cast<MyClass*>(user_data) to cast void *user_data to MyClass *. In other words, use static_cast and not reinterpret_cast<> to cast from void pointer to an object pointer.

Feedback is welcome!

5 Likes
2

Can we extend that for the case when ints are cast to enums and back (Compiler Explorer)?

Makes sense, although the more general rule should be to use reinterpret_cast only when static_cast does not work (e.g. casting ModifierData * to DecimateModifierData *).

3

Can we extend that for the case when ints are cast to enums and back

Sure. I’ve updated the proposal.

Makes sense, although the more general rule should be to use reinterpret_cast only when static_cast does not work (e.g. casting ModifierData * to DecimateModifierData * ).

Indeed. The intent of that typical case demonstration was to ensure that we use static_cast when casting from the void *. In this case both reinterpret_cast and static_cast will compile, but the latter seems more correct. Casting types like in your example should fall under the general proposal which says to use reinterpret_cast. Do you have some suggestions for wording to make it more clear in the proposal?

4

Maybe something like this?

  • For other type conversions use static_cast when possible and reinterpret_cast or const_cast otherwise.
5

I like it! Updated the proposal.

6

Functional style casts have a down-side that they share a syntactic style with function calls, So if your browsing some code, (SomeMethod)value is distinct from SomeMethod(value) which reads as if it may be calling a constructor instead of primitive type conversion. I don’t see it as unacceptable, but it is a minor down side.

Overall the proposal seems fine though.

1 Like
7

I like the proposal. It’s nice that the functional casts look more like a constructor or a shortened static cast, and the order they’re evaluated in is more clear.

Thanks for taking the time to write this.

8

Is only used for arithmetic types, which are unlikely to collide with method names as commonly those are a keyword :slight_smile:

9

What about multi-word C-style casts, like (unsigned int)value? Those are incompatible with the functional-style casts. Would they translate to static_cast<unsigned int>(value) (same type, different cast) or to uint(value) (different type, same cast)?

10

Good question. I’d imagine they to translate to uint(value).

11

In that case, +1 from me! I like the functional casts much more than the traditional C style.

12

Our style guide already has this:

When using unsigned integers, always use uint8_t, uint16_t, uint32_t or uint64_t.

So unsigned int shouldn’t really be used anyway.

3 Likes
13

Thanks for the feedback. It is now part of the Style Guide.

3 Likes
14

Committed functionality to automate C to C++ style cast conversion.

Example command that runs over the entire code-base:

./source/tools/utils_maintenance/code_clean.py \
    /your/build/dir \
    --match ".*/.*\.c[a-z]+" \
    --edit use_function_style_cast

Currently it only handles integer types.

I’m still skeptical about this convention for enum types, especially when there aren’t currently constraints on enum naming.

IndexRange(var) calls a constructor where wmOperatorCallContext(var) casts a number. The difference isn’t at all obvious. It could be argued the difference shouldn’t matter - but I think it’s not so great that casts/function-calls are easily confused as function calls may have side-effects or perform allocations … which is useful to know when reading/debugging code.