Differential D7046
CodeCleanup: Added eImBufFlags enum Authored by Jeroen Bakker (jbakker) on Mar 6 2020, 2:46 PM.
Details 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.
Diff Detail
Event Timeline
Comment Actions Solved issues from code review
Comment Actions 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; 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. So all in all the current patch adds more mess than it actually attempts to solve. Comment Actions 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. Comment Actions
That isn't what I am proposing.
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. Comment Actions 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. Comment Actions 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. Comment Actions @Brecht Van Lommel (brecht), to followup discussion here and follow the "stop talking and show me the code" typical FLOSS approach: 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. 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. Comment Actions 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. Comment Actions 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? Comment Actions 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.
Comment Actions Seem that I don't have time to fix this patch. It is currently outdated. Anyone who wants can pick up this cleanup. | |||||||||||||||||||||||||||||||||||||||||||