Page MenuHome

CodeCleanup: Added eImBufFlags enum
AcceptedPublic

Authored by Jeroen Bakker (jbakker) on Fri, Mar 6, 2:46 PM.

Details

Summary

By adding enums to debuggers are able to decode the value underneath to an understandable form. This saves time when developing or debugging.

This patch introduces an enum for ImBuf.flags. ImBuf is used in CPP-code so we needed a mechanism to support C-type flags in CPP.
The BLI_ENUM_FLAG_OPERATORS macro generates CPP-operations for a given enum.

Diff Detail

Repository
rB Blender
Branch
imbuf_enum (branched from master)
Build Status
Buildable 7042
Build 7042: arc lint + arc unit

Event Timeline

source/blender/imbuf/IMB_imbuf_enums.h
62

Windows has a macro (DEFINE_ENUM_FLAG_OPERATORS) that can generate these operators. Perhaps we should add a blenlib wrapper and implement it for other plaforms.

Brecht Van Lommel (brecht) added inline comments.
source/blender/imbuf/IMB_imbuf_enums.h
62

Seems reasonable to add that to BLI.

Jeroen Bakker (jbakker) marked an inline comment as done.

Added a macro to generate CPP operators for C enums

Synced with master

Brecht Van Lommel (brecht) requested changes to this revision.Mon, Mar 9, 2:05 PM
Brecht Van Lommel (brecht) added inline comments.
intern/ghost/intern/GHOST_EventDragnDrop.h
29 ↗(On Diff #22533)

Put this in the IMB header. It should generally be possible to include headers by themselves.

source/blender/blenlib/BLI_utildefines.h
803

I would name this BLI_ENUM_FLAG_OPERATORS.

835

Add an #else here and define an empty macro. That way we don't have to add #ifdef __cplusplus wherever this is used.

source/blender/imbuf/IMB_imbuf_enums.h
25

Don't make this include conditional on C/C++.

This revision now requires changes to proceed.Mon, Mar 9, 2:05 PM

Made macro work under C and CPP. When compiled in C it generates nothing.

Jeroen Bakker (jbakker) marked 4 inline comments as done.

Solved issues from code review

  • include BLI_utildefines in IMB_imbuf_enum
  • renamed macro
Jeroen Bakker (jbakker) edited the summary of this revision. (Show Details)Mon, Mar 9, 3:34 PM
This revision is now accepted and ready to land.Mon, Mar 9, 3:41 PM
Sergey Sharybin (sergey) requested changes to this revision.Tue, Mar 10, 11:20 AM

Having proper type for individual type is fine and something we should be moving towards to.

But the idea behind BLI_ENUM_FLAG_OPERATORS is wrong as it defines operators which are not a closed group operations: the domain of result is different from inputs, even though they all have the same type.

Imagine you have something: eImBufFlags flag = IB_multilayer;
For this you can easily do:

switch(flag) {
  case IB_multilayer: /* ... */; break;
  case IB_rect: /* ... */; break;
  case IB_rect_float: /* ... */; break;
}

And this is something you naturally expect from an enumeration type of variable.

This is no longer a case if you allow eImBufFlags flag = IB_multilayer | IB_rect;

In C++ code you could define a type like using ImBufBitflags = BaseBitFlags<eImBufFlags>; and have bit-wise operations implemented for ImBufBitflags and that would all make sense.
But you can't do this in C and all this idea of using single type to hold individual flags and bitmask of them causes confusion and opens doors to potential real issues.

So all in all the current patch adds more mess than it actually attempts to solve.

This revision now requires changes to proceed.Tue, Mar 10, 11:20 AM

Am I understanding correct that you are proposing to use a different data type in C and C++. And then to do some (explicit?) conversion in C++ whenever a C data structure or function is used?

I can see how that prevents error in C++ code, but I'm not sure it's really worth the code complexity and potential confusion from having two names for one data type.

Am I understanding correct that you are proposing to use a different data type in C and C++. And then to do some (explicit?) conversion in C++ whenever a C data structure or function is used?

That isn't what I am proposing.

and potential confusion from having two names for one data type.

This patch have two datatypes under the same name. This is what i am trying to prevent from having.

In the world of only C++ there is a nice solution. In the world where same code needs to be accessed from C and C++ there is probably no better solution than just to typedef int under a more semantic name.

I think this is a better solution than typedef int, because it prevents errors where you accidentally assign something to the completely wrong data type, maybe in a struct or in a long list of function parameters.

Leaving it as int does not preven using switch, and if you do use switch you've probably misunderstood the meaning of the data structure entirely rather than made a mistake that can slip through.

This isn't about forbidding switch on a type, you can always use whatever via either explicit or implicit cast. This is about having clarity of how to handle data of a specific type.

@Brecht Van Lommel (brecht), to followup discussion here and follow the "stop talking and show me the code" typical FLOSS approach:

1// GCC case: gcc-9 -Wall -Wextra
2// Clang case: clang-9 -Wall -Wextra
3
4typedef enum MyEnum {
5 A,
6 B,
7 C,
8} MyEnum;
9
10typedef enum MyOtherEnum {
11 D,
12 E,
13 F,
14} MyOtherEnum;
15
16void foo(MyEnum a) {
17 (void)a;
18}
19
20void bar(MyOtherEnum a) {
21 (void)a;
22}
23
24void baz(void* a) {
25 (void)a;
26}
27
28int main() {
29 foo(10);
30 foo(A | D);
31
32 bar(10);
33 bar(A | D);
34
35 foo(D); // Fine GCC (implicitly from MyEnum -> int -> MyOtherEnum).
36 // Warning in Clang.
37 bar(A); // Fine GCC (implicitly from MyOtherEnum -> int -> MyEnum).
38 // Warning in Clang.
39
40 baz(A); // Fine in GCC (A is value of 0, which can be implicitly converted to pointer).
41 // Warning in Clang.
42 baz(D); // Fine in GCC (A is value of 0, which can be implicitly converted to pointer).
43 // Warning in Clang.
44
45 baz(B); // Warning in GCC,
46 // Warning in Clang.
47 baz(C); // Warning in GCC.
48 // Warning in Clang.
49
50 return 0;
51}

Turns out GCC is rather pathetic in detecting misuse, Clang is somewhat better. But even the latter one will only catch very trivial case, and will not be any of help for actual use of bitmask field (since that is always implicitly int when you do foo | bar, and it doesn't matter if foo and bar are coming from different enumerators, and in ImBuf example it's usually multiple flags assigned at same time).

So the claim "it adds safety" is only valid in very small percentage of cases (it's rather small in the particular codebase this patch is touching). But this safety is achieved by abusing more modern revisions of language.
I'd rather have proper use of commonly agreed data types rather than having an illusion of safety.

Sure, feel free to override my concern, but this isn't type of a change I agree with.

P.S. Just realized the code was doing | for enum items of value 0. Double-checked it separately, both GCC and Clang do NOT generate warning on C | F.

This revision is now accepted and ready to land.Tue, Mar 10, 3:32 PM

Regarding ghost using blenlib.

In the past we have kept ghost separated, this patch adds blenlib/ includes for generic headers.

While the header it's self isn't a problem to use, this means future commits may use blenlib in ghost.

There are some advantages in minimizing our own inter-dependencies, so I'd rather avoid this if possible.

If we only need blenlib for BLI_compiler_compat.h, We could move this header into a header-only intern/.../ directory, possibly BLI_compiler_attrs.h can be renamed & moved there too.

The reason is for BLI_utildefines.h, which isn't used by ghost so we could add a stub or a pre-define. Both I don't like as it hides the real issue.

But basically we could ask the question why we use an ImBuf for storing a straight forward bitmap representation. If we will add a GHOST_Bitmap to represent a bitmap on a clipboard. Looking for occurrences this is only used on Mac builds and there the implementation seems limited and broken. But that could be a next code clean-up project?

We should not add a GHOST_Bitmap and associated code duplication just to make this code cleanup work. Then we are making things worse.

GHOST should be able to use blenlib, imbuf and similar libraries. I don't see the purpose of duplicating code in GHOST, it's not like that library is being used by another project. GHOST and most libraries in intern/ really should be in source/blender, there is no point making things harder for ourselves by keeping those isolated. Actually moving them is not simple because it breaks branches and patches, but I have no problem with them using blenlib.

Ray molenkamp (LazyDodo) added inline comments.
source/blender/blenlib/BLI_utildefines.h
841

30+ line un-debuggable macro, is there any way this can be turned into a template so it is atleast debuggable?