Arbitrary inner type in named impl key #4
Closed
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
| /// that can be used as a **part** of monomorphized/instantiated thunk names | ||
| /// (e.g. `cxxbridge1$box$org$rust$Struct$alloc`). | ||
| /// | ||
| /// Not all type names can be mangled at this point - `None` will be returned |
There was a problem hiding this comment.
Because it's not yet implemented, or some cases are supposed to be handled elsewhere?
There was a problem hiding this comment.
Thanks! I can expand this comment to explain better.
syntax/symbol.rs
Outdated
| // [an injection](https://en.wikipedia.org/wiki/Injective_function). | ||
| for c in self.0.chars() { | ||
| match c { | ||
| '_' => f.write_str("__")?, |
There was a problem hiding this comment.
Does this end up in symbol names used elsewhere? Or is it purely internal?
(I'm wondering if we need to worry since __ is traditionally reserved for the implementation in C++)
There was a problem hiding this comment.
Thanks! IIRC this would end up being used as part of identifiers used in the generated Rust code. So, yes - using _u as a substitution is probably safer.
This commit refactors `write_...` functions to enable formatting a C++ representation of a `Type` into any generic `impl std::fmt::Write` instead of only supporting formatting into an `OutFile`. This is then used to provide `fn stringify_type`. The new function is not used in this commit, but will become quite handy in a follow-up commit when `inner` type of generics/templates may not necessarily be a simple `std::fmt`-friendly `Type::Ident`. Without the refactored `write_...` APIs, the follow-up would have to split *single* `writeln!` invocations (such as the ones in `fn write_shared_ptr` or `fn write_cxx_vector`) into *multiple* ones - e.g. into: `write!` + `write_type` + `write!` + `write_type` + `writeln!`. The new function is a little bit redundant wrt the already existing `trait ToTypename`, but we can't replace that trait just yet: * The trait has slightly different semantics (formatting not the whole type represented by `self`, but only formatting the inner `T`). * The trait works with `Ident` rather than `Type` (this will change in the follow-up commit mentioned above).
Before this commit `NamedImplKey` could only represent the inner type as
`rust: &'a Ident`. For example, it could represent `Vec<Foo>` but could
not represent `Vec<Box<Foo>>` where the inner type (`Box<Foo>` in our
example) is not a simple identifier.
After this commit `NamedImplKey` is refactored to support an arbitrary
inner type. Note that (to simplify and to minimize risks associated
with this commit) this new ability is not actually used at this point -
it is planned to be used in follow-up commits to incrementally relax
generic type argument restrictions in `syntax/check.rs`.
This commit is quite big, but it seems difficult to extract some changes
to smaller, separate commits, because all of the changes stem from the
refactoring of the `NamedImplKey`. At a high-level this commit contains
the following changes:
1. `syntax/instantiate.rs`: Changing `pub rust: &'a Ident` field of
`NamedImplKey` to `pub inner: &'a Type`. This is the main/root
change in this commit.
2. `gen/src/write.rs`: supporting arbitrary inner types
when writing C++ thunks exposing instantiations/monomorphizations of
templates/generics supported by `cxx`.
* This depends on `fn stringify_type` introduced in
`gen/src/write.rs` in an earlier commit.
* Handling arbitrary inner types *in general* means that we can
delete `enum UniquePtr` which provided handling of two *specific*
inner types.
3. `macro/src/expand.rs`: supporting arbitrary inner types
when writing Rust thunks exposing instantiations/monomorphizations of
templates/generics supported by `cxx`.
* Using `#inner` instead of `#ident` may now (optionally) cover
generic lifetime arguments. This is why this commit also changes
`macro/src/generics.rs`. And this is why we can no longer need
the `ty_generics` field from `struct Impl`.
* One minor functional change here is changing the error messages
so that references to type names are generated purely in the
generated bindings, without depending on `fn display_namespaced`.
4. `syntax/mangle.rs`: supporting mangling of individual types. This
helps to:
* Support the (long-term, not-yet-realized) high-level goal of
actually allowing and using arbitrary inner types
* Deduplicate mangling code details that were somewhat duplicated
in `macro/src/expand.rs` and `gen/src/write.rs`.
5. `syntax/types.rs`: Supporting arbitrary inner types in
* `fn is_maybe_trivial`
* `fn is_local` (this function supports an earlier refactoring
that changed how `cxx` decides whether to provide an *implicit*
impl of a given generic/template instantiation/monomorphization)
anforowicz
force-pushed
the
arbitrary-inner-type-in-named-impl-key
branch
from
1c784bf to
795da6d
Compare
|
Thanks for the feedback. Let me close this PR - I'll replace it with an upstream PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
@zetafunction, can you PTAL before I open a PR against the upstream repo?