Field visibility callback

[jethrogb]

Add a callback mechanism for determining field visibility. This PR includes 2 other related (breaking) behavior changes in the prior commits:

• Use default visibility for padding fields Previously, padding fields were always pub. Now, they follow the default visibility for the type they're in.
• Compute visibility of bitfield unit based on actual field visibility A bitfield unit field and its related functions now have their visibility determined based on the minimum of the default visibility and the actual visibility of the bitfields within the unit. private < pub(crate) < pub.

[amanjeev]

Hi @jethrogb thank you for this PR. Could you please add a couple of tests showing the interaction of this work with the flag --enable-cxx-namespaces. In our understanding that should take precedence and showing it does not break is nice, when used along with this one. What do you think?

[jethrogb]

I assume you mean respect-cxx-access-specs instead of enable-cxx-namespaces. If you don't, could you please provide a code example of what you'd like tested?

I'm not sure what you mean by “that should take precedence and showing it does not break”, the callback takes precendence over whatever is inferred from the type, just like the annotations already do. I've added some tests in private_fields.hpp.

[pvdrz]

I assume you mean respect-cxx-access-specs instead of enable-cxx-namespaces. If you don't, could you please provide a code example of what you'd like tested?

I'm not sure what you mean by “that should take precedence and showing it does not break”, the callback takes precendence over whatever is inferred from the type, just like the annotations already do. I've added some tests in private_fields.hpp.

Hmmm... Right now default_visibility is ignored if respect_cxx_access_specs is enabled (and if it isn't, we wrote the docs wrong 😅). The big question here is if this callback should take precedence over both this options or not. I find two approaches that are reasonable from different points of view:

• respect_cxx_access_specs cannot be overriden if enabled: This makes sense because modifying the visibility of fields would let you access items that aren't part of the public API, which sounds like a recipe for bad things to happen.
• This callback can override respect_cxx_access_specs: This makes sense as it would be the most powerful option and you could argue that it is not bindgen's business to tell users not to do stuff that could cause trouble.

I'm more inclined towards the first option but I'd be happy to hear an argument about any other option in any case.

[jethrogb]

As is apparent from af66f4a, and as I explained in my comment, the behavior of overriding respect-cxx-access-specs is already there.

[pvdrz]

Oh OK I missed the annotations part. Yes, so this feature has the same precedence as the annotations. My question is if that's desirable or not because it somehow feels "wrong" to enable --respect-cxx-access-specs to just ignore it with this callback.

You could argue that annotations also ignore this but at least annotations are part of the input taken by bindgen which means the person who wrote the C code had the explicit intention of overriding the visibility of a field when running bindgen for these headers.

I'm not saying this reason is strong enough but it should be properly stated that anyone using this callback when --respect-cxx-access-specs is enabled are "on their own" as they aren't protected of accessing fields they should not. Which I imagine is the original intent of --respect-cxx-access-specs.

I'll review the actual code in a bit and give you a proper review. Just wanted to clarify this point.

[pvdrz]

This looks awesome! I requested some small changes but the core logic looks great. Could you also document all these changes in the changelog? Thanks

[jethrogb]

I've made the requested changes.

it somehow feels "wrong" to enable --respect-cxx-access-specs to just ignore it with this callback.

People generally use these callbacks as a "last resort" when they find bindgen doesn't quite do what they wanted. As such I think it's correct to override the more generic mechanism.

[pvdrz]

Hey! the test suite broke due to an unrelated issue that has already been fixed in the main branch, could you rebase your PR against it so we can be sure the tests are passing?