Map Features objects in bindings #795
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## main #795 +/- ##
==========================================
- Coverage 90.75% 90.74% -0.01%
==========================================
Files 44 44
Lines 24075 24075
==========================================
- Hits 21849 21848 -1
- Misses 2226 2227 +1
Continue to review full report at Codecov.
|
TheBlueMatt
force-pushed
the
2021-02-features-bindings
branch
from
0c16439 to
ee3f42f
Compare
valentinewallace
approved these changes
Feb 17, 2021
| #[no_mangle] | ||
| pub extern "C" fn ChannelPublicKeys_write(obj: &ChannelPublicKeys) -> crate::c_types::derived::CVec_u8Z { | ||
| crate::c_types::serialize_obj(unsafe { &(*(*obj).inner) }) | ||
| crate::c_types::serialize_obj(unsafe { &*unsafe { &*obj }.inner }) |
There was a problem hiding this comment.
OOC, is the 2nd unsafe redundant or necessary?
There was a problem hiding this comment.
No, its just printed in the serialize_obj call generation because enums need it, though for structs its duplicative (but harmless).
jkczyz
approved these changes
Feb 18, 2021
In traits with associated types which are returned in generics (ie
`trait T { type A: B; fn c() -> Result<Self::A, ()> {} }`), we
created a new generic mapping with the local type name (in this
case A) instead of using the real type (in this case B). This is
confusing as it results in generic manglings that don't reference
the real type (eg `LDKCResult_ChanKeySignerDecodeErrorZ`) and
may have multiple generic definitions that are identical.
Instead, we now use the final ident in the resolved mapping. The
biggest win is `LDKCResult_ChanKeySignerDecodeErrorZ` changing to
`CResult_ChannelKeysDecodeErrorZ`. However, there are several types
where `secp256k1::Error` was imported as `SecpError` and types like
`LDKCResult_SecretKeySecpErrorZ` are now
`LDKCResult_SecretKeyErrorZ` instead. Still, the type of the error
field remains `LDKSecp256k1Error`, which should avoid any confusion.
Instead of walking individual rust files and reading the AST from those, we instead call `RUSTC_BOOTSTRAP=1 cargo rustc --profile=check -- -Zunstable-options --pretty=expanded` and let it create one giant lib.rs which we can parse as a whole. This allows us to parse a post-macro crate, working with structs and functions created inside macros just fine. It does require handling a few things that we didn't previously, most notably Clone via `impl ::core::clone::Clone` blocks instead of just looking for `#![derive(Clone)]`. This ends up resolving a few types slightly differently, resulting in different bindings, but only in ways which don't impact the runtime.
This will allow us to reuse the same ident-resolution logic both during the initial AST pass and during the second conversion pass.
This allows an ImportResolver to handle more than imports, allowing ident resolution of objects constructed in the same module.
This removes some redundant logic and ensures we handle more import-resolution cases in both contexts.
`Result` is in the standard prelude, so no need to ever use it.
Sadly, returning a Features<T> in the `impl Futures {}` block
will confuse our new alias-impl-printing logic, as we end up
running through the normal impl-block-printing logic as if we had
an explicit `impl ConcreteFeatures` block.
We need this specifically for resolving the features::sealed::Context trait which is inside the non-pub `sealed` module.
We already map type aliases which alias private types as opaque, but we don't resolve them like we would any other opaque type, preventing conversion printing or type use.
This handles, for example, the `impl<X: Y> for Features<X>` blocks which are generic across a number of different contexts. We do so by walking the set of structs which alias Features and then walking their generic arguments to check that they meet the bounds specified in the impl block. For each alias which does, we create a dummy, explicit, `impl XFeatures` block with the same content as the original and recurse.
We now handle features properly in our own resolution, and these appear to be self-inconsistent in some cases.
TheBlueMatt
force-pushed
the
2021-02-features-bindings
branch
from
ee3f42f to
954e66e
Compare
|
Rebased on latest upstream, only change it to drop the intermediate bindings update commits and re-run a bindings update at the end. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This required some decent-sized reworkings of the bindings, but IMO all of the changes stand on their own as good cleanups or new features, so in the end its a good motivator to clean things up.