(#548) Change typedef of bool from int32 to 1-byte 'unsigned char'.

[gapisback]

In SplinterDB's public splinterdb_config{} config, we have few fields defined as 'bool' which is typedef'ed to int32 on our side. This creates compatibility problems when linking this library with other s/w which may have defined 'bool' as 1-byte field. (Offsets of fields in the splinterdb_config{} struct following 1st field defined as 'bool' changes across objects. This commit addresses this issue by defining boolean fields in public struct to be of type _Bool. This should, hopefully,reduce the risk of such incompatibilities.

(Splinter's internal definition of bool remains as int32, to avoid any other product change / compatibility issues.)

Found only one instance where 'bool' return value from routing_filter_is_value_found() had to be adjusted to fit 1-byte return value.

[netlify[bot]]

✅ Deploy Preview for splinterdb canceled.

Name	Link
🔨 Latest commit	55ddc1288ec742964724e2b9ad6a7580ee2e19a8
🔍 Latest deploy log	https://app.netlify.com/sites/splinterdb/deploys/642e210e0f461e00082961b9

[gapisback]

@rtjohnso - Triggered by the fix I discovered as being necessary in src/routing_filter.h, I am doing a code walk thru myself, cross-checking all functions that are defined to return bool, to see if other similar issues are lurking elsewhere.

I am not finding any ... hopefully, this fix in src/routing_filter.h is the ONLY place where code needs to be adjusted.

[gapisback]

@rtjohnso - I completed my code-walk through for all instances of bool in the code. I found a few straggling places where code was using hard-coded 0 or 1 for boolean variables. I pushed this commit to do some more minor clean-up.

None of this changes the logic / semantics, but will be good to make sure I get one more pair of eyes over it (lest, I'm making some silly mistakes).

[gapisback]

HI, @rtjohnso - I reworked this fix as per what we discussed this past Tue's PR meeting. Now, in public config struct all boolean fields are defined as _Bool.

The rest of the change set stays as it was before, including minor cleanup for (what was previously) one instance of a bug, and other cleanup of hard-coded uses of 0 /1 with FALSE / TRUE.

[gapisback]

@rtjohnso - It's not straightforward w/o too much work to address your suggestion of : "Don't forget to move the definition of bool from the public platform to the private platform."

There are some cascading dependencies which cause compilation problems.

For instance, in include/splinterdb/public_platform.h we explicitly document a dependency that:

 14 The external definitions include:
 15 - uint64, which is used for buffer sizes, etc.
 16 - bool, typedef'd to int32 on linux

I had tried to move this typedef to src/platform_linux/platform.h and this immediately results in a compilation failure:

Compiling            src/btree.c                                        [build/release/obj/src/btree.o]
In file included from include/splinterdb/data.h:23,
                 from src/btree_private.h:14,
                 from src/btree.c:4:
include/splinterdb/public_util.h:30:15: error: unknown type name ‘bool’
   30 | static inline bool
      |               ^~~~

This is (partly) why in my original version of the fix, I had retained the location of typedef bool to be in include/splinterdb/platform_linux/public_platform.h but changed it to unsigned char.

Unless I'm missing something, I suspect to implement this new approach (of using _Bool in extern headers, and to use bool as int32 for internal usages) will need some .h file reorganization.

I'm not sure it's a good idea to take that on right now. It should be a separate item.

[rtjohnso]

If we leave our definition of bool in the public header, then won't you still have your postgres problem?

Just change the static inline bool functions in public headers to use _Bool instead.

Best, Rob

On Fri, Mar 24, 2023 at 5:18 PM gapisback @.***> wrote:

@rtjohnso https://github.com/rtjohnso - It's not straightforward w/o too much work to address your suggestion of : "Don't forget to move the definition of bool from the public platform to the private platform."

There are some cascading dependencies which cause compilation problems.

For instance, in include/splinterdb/public_platform.h we explicitly document a dependency that:

14 The external definitions include: 15 - uint64, which is used for buffer sizes, etc. 16 - bool, typedef'd to int32 on linux

I had tried to move this typedef to src/platform_linux/platform.h and this immediately results in a compilation failure:

Compiling src/btree.c [build/release/obj/src/btree.o] In file included from include/splinterdb/data.h:23, from src/btree_private.h:14, from src/btree.c:4: include/splinterdb/public_util.h:30:15: error: unknown type name ‘bool’ 30 | static inline bool | ^~~~

This is (partly) why in my original version of the fix, I had retained the location of typedef bool to be in include/splinterdb/platform_linux/public_platform.h but changed it to unsigned char.

Unless I'm missing something, I suspect to implement this new approach (of using _Bool in extern headers, and to use bool as int32 for internal usages) will need some .h file reorganization.

I'm not sure it's a good idea to take that on right now. It should be a separate item.

— Reply to this email directly, view it on GitHub https://github.com/vmware/splinterdb/pull/549#issuecomment-1483623740, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA45FROW2WPUH2SVAF7ZEGTW5Y2T7ANCNFSM6AAAAAAWBW6VDU . You are receiving this because you were mentioned.Message ID: @.***>

[gapisback]

If we leave our definition of bool in the public header, we won't have the field-offset mismatch when integrating with Postgres because all the boolean fields in the public splinterdb_config{} have been changed to _Bool.

So irrespective of whether struct is compiled with our library or PG-glue-code, we should see the same field definition.

I meant to say: The offsets of the boolean fields are protected by use of _Bool.

Even if bool remains in public headers, it would / should not affect PG's usage as bool will be used only for our internal functions returning boolean value.

Let me give it a shot to also change the single instance of bool slice_is_null in public_util.h to see if I can get compilation working.

[gapisback]

@rtjohnso - I amended the code to use _Bool for few methods whose protos are in public_util.h . This allows me to relocate typedef bool int32 to platform.h and to go through compilation.

(Removed the newly added test-case.)

Let me know if you find this new version of the fix acceptable.