BLI: Replace some macros with inlined functions for C++ #108041

Sergey Sharybin · 2023-05-18T11:51:54+02:00

Sergey Sharybin commented

2023-05-18 11:51:54 +02:00

Covers the macro ARRAY_SIZE() and STRNCPY.

The problem this change is aimed to solve it to provide cross-platform
compiler-independent safe way pf ensuring that the functions are used
correctly.

The type safety was only ensured for GCC and only for C. The C++
language and Clang compiler would not have detected issues of passing
bare pointer to neither of those macros.

Now the STRNCPY() will only accept a bounded array as the destination
argument, on any compiler.

The ARRAY_SIZE as well, but there are a bit more complications to it
in terms of transparency of the change.

In one place the ARRAY_SIZE was used on float3 type. This worked in the
old code because the type implements subscript operator, and the type
consists of 3 floats. One would argue this is somewhat hidden/implicit
behavior, which better be avoided. So an in-lined value of 3 is used now
there.

Another place is the ARRAY_SIZE used to define a bounded array of the
size which matches bounded array which is a member of a struct. While
the ARRAY_SIZE provides proper size in this case, the compiler does not
believe that the value is known at compile time and errors out with a
message that construction of variable-size arrays is not supported.

Solved by converting the field to std::array<> and adding dedicated
utility to get size of std::array at compile time. There might be a
better way of achieving the same result, or maybe the approach is
fine and just need to find a better place for such utility.

Surely, more macro from the BLI_string.h can be covered with the C++
inlined functions, but need to start somewhere.

There are also quite some changes to ensure the C linkage is not
enforced by code which includes the headers.

Covers the macro ARRAY_SIZE() and STRNCPY. The problem this change is aimed to solve it to provide cross-platform compiler-independent safe way pf ensuring that the functions are used correctly. The type safety was only ensured for GCC and only for C. The C++ language and Clang compiler would not have detected issues of passing bare pointer to neither of those macros. Now the STRNCPY() will only accept a bounded array as the destination argument, on any compiler. The ARRAY_SIZE as well, but there are a bit more complications to it in terms of transparency of the change. In one place the ARRAY_SIZE was used on float3 type. This worked in the old code because the type implements subscript operator, and the type consists of 3 floats. One would argue this is somewhat hidden/implicit behavior, which better be avoided. So an in-lined value of 3 is used now there. Another place is the ARRAY_SIZE used to define a bounded array of the size which matches bounded array which is a member of a struct. While the ARRAY_SIZE provides proper size in this case, the compiler does not believe that the value is known at compile time and errors out with a message that construction of variable-size arrays is not supported. Solved by converting the field to std::array<> and adding dedicated utility to get size of std::array at compile time. There might be a better way of achieving the same result, or maybe the approach is fine and just need to find a better place for such utility. Surely, more macro from the BLI_string.h can be covered with the C++ inlined functions, but need to start somewhere. There are also quite some changes to ensure the C linkage is not enforced by code which includes the headers.

👍 1

Sergey Sharybin commented

2023-05-18 11:52:09 +02:00

@blender-bot build

Sergey Sharybin changed title from ~~BLI: Replace some macros with inlined functions for C++~~ to WIP: BLI: Replace some macros with inlined functions for C++

2023-05-18 11:59:21 +02:00

Sergey Sharybin force-pushed cxx_avoid_macro from bbb6fa2cb1 to 30cc4be8d4

2023-05-18 14:04:15 +02:00

Compare

Sergey Sharybin commented

2023-05-18 14:04:30 +02:00

@blender-bot build

Sergey Sharybin changed title from ~~WIP: BLI: Replace some macros with inlined functions for C++~~ to BLI: Replace some macros with inlined functions for C++

2023-05-18 14:37:38 +02:00

Sergey Sharybin requested review from Jacques Lucke 2023-05-18 14:37:48 +02:00

Sergey Sharybin requested review from Hans Goudey 2023-05-18 14:37:55 +02:00

Campbell Barton commented

2023-05-19 00:53:56 +02:00

The type safety was only ensured for GCC and only for C. The C++
language and Clang compiler would not have detected issues of passing
bare pointer to neither of those macros.

While true, the ARRAY_SIZE() macro reports a -Wsizeof-pointer-div warning from incorrect use with GCC/C++, nevertheless a portable C++ solution is best.

> The type safety was only ensured for GCC and only for C. The C++ language and Clang compiler would not have detected issues of passing bare pointer to neither of those macros. While true, the `ARRAY_SIZE()` macro reports a `-Wsizeof-pointer-div` warning from incorrect use with GCC/C++, nevertheless a portable C++ solution is best.

Jacques Lucke approved these changes 2023-05-22 14:23:53 +02:00

source/blender/blenlib/BLI_string.h

						
				@ -638,0 +646,4 @@

				 *

				 * Returns the dst.

				 */

				template<size_t N> inline char *STRNCPY(char (&dst)[N], const char *src)

Jacques Lucke commented

2023-05-22 14:19:25 +02:00

Think it would be better to define the C and C++ version of STRNCPY next to each other, not with 100 lines in between.

Think it would be better to define the C and C++ version of `STRNCPY` next to each other, not with 100 lines in between.

Sergey Sharybin commented

2023-05-22 15:51:20 +02:00

I was thinking about it.

The reason I did not go that route is because the surrounding C linkage specializer. We'll either need to break it and re-enter again (which is a bit too verbose), or do a bigger change to move the macro outside of the C linkage block (which would mean changes in the way BLI_string_debug_size is defined).

It felt that it is safer to not make it easier to use wrong linkage for functions declared in the middle of a a macro (which happens a lot in Blender), and just let the current macro fade out with the rest of the C code.

How strongly you feel about avoiding the gap? And how would you organize the macro and linkage specializer?

I was thinking about it. The reason I did not go that route is because the surrounding C linkage specializer. We'll either need to break it and re-enter again (which is a bit too verbose), or do a bigger change to move the macro outside of the C linkage block (which would mean changes in the way `BLI_string_debug_size` is defined). It felt that it is safer to not make it easier to use wrong linkage for functions declared in the middle of a a macro (which happens a lot in Blender), and just let the current macro fade out with the rest of the C code. How strongly you feel about avoiding the gap? And how would you organize the macro and linkage specializer?

Jacques Lucke commented

2023-05-22 16:00:08 +02:00

Don't feel strong enough about it now to look into it in more detail.