Emit fully qualified namespaces #369
Closed
Conversation
This file contains 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
This change is the first step towards including a namespace into all(*) C++ identifiers. In this commit, there is no intentional functional difference: a single namespace continues to apply to the entire #[cxx::bridge]; it just happens to be stored differently. (*) = This change aims to include a namespace for: * C++ types * C++ functions but not for: * C++ fields * C++ enum variants * Rust types/functions/anything. A 'QualifiedIdent' struct is used for the first category of things; plain old proc_macro2::Ident for the latter. It may be that methods are being treated incorrectly, because they're functions yet should not have a globally-scoped name. A subsequent commit may need to fix that. At the moment, the Namespace (included in each QualifiedIdent) is ruthlessly cloned all over the place. As a given namespace is likely to be applicable to many types and functions, it may save significant memory in future to use Rc<> here. But let's not optimise too early. This commit does not currently output the namespace name in any different ways or different places (intentionally). syntax/mod.rs has a Boolean constant USE_FULLY_QUALIFIED_NAMES which will enable that, and it completely breaks everything. Substantial changes to both Rust and C++ code generation will be required to use this. It may be desirable to move the QualifiedIdent code out of mod.rs. The rough sequence of commits in mind are: 1) This commit (just stores the namespaces) 2) Use them when writing Rust and C++ code as necessary 3) Allow the current namespace attribute to be overridden by finer-grained namespace attributes on individual functions 4) Consider allowing sub-mods which can also have a namespace attribute. 5) Future: Introduce a resolution pass which tries to resolve a given identifier to a target symbol, after all symbols have been read. Support 'use' statements.
This change: * Outputs fully-qualified C++ names wherever possible or appropriate; * Supports outputting content belonging to different namespaces. This change should have no functional effect, because at the moment the only way to set up a namespace is still to configure the overall [cxx::bridge] with a namespace attribute. However, in theory, it should allow finer-grained namespaces to be supported in subsequent commits. In theory, it should be possible for items in one namespace to correctly reference another thanks to the fully qualified names.
These two types are almost, but not quite, the same: * QualifiedName is a C++ name in a particular namespace. * Symbol is an already-mangled name. This commit makes this relationship a bit more clear. As Symbols (with the same mangling) are used for both include guards and actual exported symbol names, that distinction is also dropped from the QualifiedName API.
adetaylor
force-pushed
the
always-emit-namespaces
branch
from
2b60a43 to
28f5671
Compare
7 tasks
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 is some work towards #353.
It's not complete. I think it does the hard bits:
But we do not currently have any way to set different namespaces, and there are therefore no extra tests yet.
So it feels like it's about half the job (if we exclude the later need to resolve
usestatements).I'm raising this PR now because it's already pretty complicated, and there are (at least) the following areas where I think rework may be required and I would like your comments.
QualifiedIdents instead ofIdents all over theApiwas what you had in mind. But per the 75f435e commit comment, it would be great to understand that I've qualified the names for roughly the right set of things. (I do expect to find some anomalies when we come to add tests).parse.rsrecognizes primitives and built-ins, which should therefore not have the current namespace prepended. This is an interim step until we supportuse, but I'd be interested in your views about whether it's about right meanwhile (I do think the list of built-ins should probably be abstracted somewhere, at least.)Symboland its relationship toQualifiedIdent. I think it's OK to have a distinct type (QualifiedIdent= C++ name with namespace;Symbol= mangled version of same) but I feel as though I should now be able to rip out a ton ofSymbolcode and I can't.QualifiedIdentcode should go into aqualified_ident.rsfile.As ever, feel free to tinker with these things directly or give me your wisdom so I can do follow-up work!