Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Build warning - cast from pointer to integer of different size #133

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Build warning - cast from pointer to integer of different size #133

wants to merge 2 commits into from

Conversation

DimitriPapadopoulos
Copy link
Contributor

Might fix #132.

This was referenced Aug 28, 2021
Merged
Open
@DimitriPapadopoulos DimitriPapadopoulos force-pushed the pointer_integer_cast__private_api branch 5 times, most recently from d509b2a to a04c7c6 Compare August 28, 2021 20:45
@LB-- LB-- added this to the v1.2.0 milestone Sep 3, 2021
LB--
LB-- requested changes Sep 3, 2021
View reviewed changes
Copy link
Member

@LB-- LB-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking alright so far, but the define will have to be changed since the standard(s) reserve(s) all names that contain two or more consecutive underscores (or also those that start with an underscore and then a capital letter). Changing the double underscore to a single underscore should suffice.

@DimitriPapadopoulos
Copy link
Contributor Author

Ah, I knew about the leading underscore, not about the consecutive underscores. I can fix that.

Is the code maybe getting too complex? Should we maybe live with the additional ._p or .u in the API?

Also should I keep _p or maybe use plain u for the union name?

@DimitriPapadopoulos
Copy link
Contributor Author

I have also renamed _p to _u. The leading _ is sufficient to denote this is a private field, the p instead of the u doesn't bring anything but confusion.

@jamesamcl
Copy link
Collaborator

It might be even easier to just copy the whole json_value struct into json.c and make a json_value_internal. Because the ifdefs you added are in the C++ helpers, which aren't available in the json_value visible to json.c anyway.

Is the code maybe getting too complex? Should we maybe live with the additional ._p or .u in the API?

Nahh that would be admitting defeat ;-)

@DimitriPapadopoulos
Copy link
Contributor Author

DimitriPapadopoulos commented Sep 15, 2021 •
edited

Creating a json_value_internal will imply casting back and forth. I fear this could be more horrible.

Also, if some day we need to modify json_value, we might forget to update json_value_internal.

Finally, I feel it's more complicated for new maintainers to understand two distinct structs instead of one struct with #ifdef's.

@DimitriPapadopoulos
Copy link
Contributor Author

The C++ helpers are visible by json.c if json.c is compiled by a C++ compiler. We have tests that do exactly that.

LB--
LB-- approved these changes Oct 30, 2021
View reviewed changes
Copy link
Member

@LB-- LB-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me. I definitely agree that casting back and forth between a public and private structure could lead to all sorts of issues with maintainability. In either case though, the end result is some sort of violation of one thing or another. I'll have to do some testing to see how this plays out with ODR violations, but in theory it should be okay.

@LB-- LB-- self-assigned this Oct 30, 2021
@DimitriPapadopoulos DimitriPapadopoulos changed the title WIP: build warning - cast from pointer to integer of different size Build warning - cast from pointer to integer of different size Oct 31, 2021
@jamesamcl
Copy link
Collaborator

Looks like this issue found its way into the VLC tracker!

https://code.videolan.org/videolan/vlc/-/issues/17626

@DimitriPapadopoulos
Fix build warning - cast from pointer to integer of different size
0c752b6 
object->values used to be a size in pass 1 and a pointer in pass 2.

A union has been added to avoid such GCC warning:
* cast from pointer to integer of different size
* dereferencing type-punned pointer will break strict-aliasing rules

The union is visible only when JSON_PRVATE_API is defined, but this
trick relies on sizeof(json_object_entry *) >= sizeof(unsigned int).

The name of the new union is "_u" instead of "u", to make clear it is
private.
@DimitriPapadopoulos
Consistent code
d2b61fc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LB-- LB-- LB-- approved these changes

Assignees

@LB-- LB--

Labels
None yet
Projects
None yet
Milestone
v1.2.0
Development

Successfully merging this pull request may close these issues.

Build warning - cast from pointer to integer of different size
3 participants
@DimitriPapadopoulos @jamesamcl @LB--