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.

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

Improvements to namespace support #353

Open
6 of 7 tasks
dtolnay opened this issue Oct 10, 2020 · 16 comments
Open
6 of 7 tasks

Improvements to namespace support #353

dtolnay opened this issue Oct 10, 2020 · 16 comments

Comments

@dtolnay
Copy link
Owner

dtolnay commented Oct 10, 2020

This issue is just to track the implementation plan laid out at the bottom of #298 (comment).

@adetaylor
Copy link
Collaborator

The second and third items on the list are probably #166.

I'm going to need to get at least half way down this list before succeeding in google/autocxx#50.

@sbrocket
Copy link
Contributor

sbrocket commented Oct 22, 2020

@adetaylor are you already working on one or more of these? If not I'm going to start working through them again.

@adetaylor
Copy link
Collaborator

Yes. I have started with the second item. I'll aim to post my current progress somewhere by the end of today, assuming I can eke out any time at all (today seems very distraction-filled)

@adetaylor
Copy link
Collaborator

OK, here's my current progress on item 2 of the list.
master...adetaylor:always-emit-namespaces

Depending on work and family commitments I may have a crack at 1, 3 and 4 tomorrow (I hope I can pinch some of #298 for the third item on the list).

Meanwhile @dtolnay if you have a chance to tell me if I'm along the right lines that'd be great!

@sbrocket
Copy link
Contributor

Ok, I'd prefer if you let me know which ones you're definitely going to take on and which you aren't as you know, so that I can take them on otherwise.

@adetaylor
Copy link
Collaborator

I have pretty much done 2 and then 1. Both are now pushed to the branch mentioned just above. Would you like to branch that branch and start to work on 3 and 4 (and most importantly, some tests for 3 and 4)? All the work on those should be in parse.rs, just filling in the existing QualifiedIdent structs from a finer-grained namespace. In theory things should now continue to work if different QualifiedIdents have different namespaces. In practice, I'm sure there will be problems!

(The last thing I'm going to be doing here is trying to remove some duplication between QualifiedIdent and Symbol, which should be orthogonal to all the rest of the work).

@adetaylor
Copy link
Collaborator

More progress here from a bit of tinkering this evening (a mess of commits which I will gradually rebase and clean up). @sbrocket please post here if you're likely to start any part of this, so we don't collide.

90% of this is tests (which of course fail horribly). The remaining 10% is that I took some of the attribute parsing code from your previous work in #298 and have plumbed it in per items 3 and 4 on the list at the top. I ended up requiring the syntax #[namespace(namespace = A::B)] rather than #[namespace = A::B] because my IDE got mildly grumpy about the former.

adetaylor/cxx@always-emit-namespaces...adetaylor:allow-namespace-override

Next steps:

  • Get these tests to pass (which would validate all the work so far)
  • Rebase and clean up these commits and raise a pull request (or append them to Emit fully qualified namespaces #369)
  • Add tests for namespacing functions, which isn't in any of these commits yet. Then make that work, clean up and PR.
  • Finally, add tests for methods on namespaced classes, clean up and PR.
  • Consider cloning Namespace objects less often.

@sbrocket
Copy link
Contributor

sbrocket commented Oct 25, 2020

I was planning on working on 3 and 4 based on your previous message but not over the weekend. If you want to just finish it that’s fine with me, I’ll check where you are on Monday.

@adetaylor
Copy link
Collaborator

OK great.

@adetaylor
Copy link
Collaborator

Hm. I think I'm going to have to take a slightly different approach here anyhow. The various maps inside Types probably need to be keyed by the un-namespaced Rust name, which I think means that the Struct and Enum types will need to store a Pair rather than a QualifiedIdent alone.

@adetaylor
Copy link
Collaborator

I've closed #369 for now as I think it needs some rework.

When we encounter a type declaration/definition, we know its C++ namespace and can fully qualify its name (that's what that PR did). However, when we encounter a type reference e.g. inside a struct, we don't know its C++ namespace. We were previously invalidly qualifying it with the currently applicable namespace.

We therefore need to do this:

  • At parse time, when we encounter a type reference, record just the Rust ident of the type.
  • Once we've parsed all types and APIs, resolve all the type references to retrieve their fully qualified C++ names. (This can be likely done in the pass we already do in types.rs)

This requires more changes to the various data structures in syntax/mod.rs which is really the core of #369, hence closing it for now.

@adetaylor
Copy link
Collaborator

OK. I got the tests to pass (bearing in mind I have only added tests for types in various namespaces, not yet functions). At the moment the branch referenced above contains many muddled commits and duplication which needs a lot of clearing up, so don't take a look yet!

@adetaylor
Copy link
Collaborator

Progress update: I've added all the tests that I said I would, and they all pass. I'm now gradually rebasing and squashing the branch into a coherent series of commits. To reiterate, this will cover points 1-4 of the plan at the top here.

@sbrocket
Copy link
Contributor

OK, if you've got 1-4 covered then I'll wait to review the PR for those, since I think that should cover everything I need for my use. I'll confirm that after you post a PR and see if there's any gaps still, and then afterwards I can loop back and consider doing 6 and/or 7 since those aren't required (for me, anyway).

@casimcdaniels
Copy link

@dtolnay Would you like help with the last item in the list? I'd like to take a crack at it if no one else is working on it to get this issue closed out.

@Xiretza
Copy link

Xiretza commented Feb 18, 2023

The last item is required for codebases that use both autocxx to generate bindings to C++ functions using extern_rust_type!(), and also manually use cxx to expose Rust functions using the same type to C++. autocxx generates a type MyRustType; in its own bridge's extern "Rust" {} block, which means you can't put type MyRustType; in the handwritten bridge's extern "Rust" {} block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants