-
Notifications
You must be signed in to change notification settings - Fork 592
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
Add Support for cpp:identifier
Metadata, and the Groundwork for xxx:identifier
Metadata in General
#3292
base: main
Are you sure you want to change the base?
Add Support for cpp:identifier
Metadata, and the Groundwork for xxx:identifier
Metadata in General
#3292
Conversation
We should not do that, just like we don't allow cs::identifier on modules in "new Slice": |
I find this sub-optimal. All our compilers generate code for only one language, so it would be nicer for the compiler to tell the Parser from the get-go "my language is xxx, please lookup xxx:identifier when resolving mapped identifiers". |
It's worth noting that modules are pretty different in IceRPC vs Ice Slice. Theoretics aside, how do you propose a C++ user fixes this then?
Or even worse, this:
|
Alright, I'll see what I can do to improve it. But I'll wait until we figure out the module situation. |
It would be logical to add a new Now, back to new Slice: why do we have It's because the semantics are different and more specialized: We should do the same for Ice Slice. The question is which metadata do we use? Say we use ["cpp:ns:Reservation::Bundles"]
module Res::Vacation
{
...
} maps to C++ namespace ["cpp:ns:Foo::Reservation"]
module Res
{
["cpp:ns:Bundles"]
module Vacation { ... }
} maps to: namespace Foo::Reservation
{
namespace Bundles
{
}
} Keep in mind the situation is simpler with new Slice since we don't support the |
Yes, but you're just describing the exact semantics of
And it will be mapped to:
Since we're targeting C++17, which supports nested namespace definitions. However... There might be a bug here with putting Using a name like And it's easier to explain Back to new Slice though, modules are quite different between Ice and IceRPC. And while the design of modules in IceRPC is simpler, I don't think that the way we handle and treat modules in Ice is wrong, just different. IceRPC uses the Java approach ( |
Also, as an aside, if we allow If you have:
It is completely backwards compatible to update this to:
|
The argument for cs::namespace in new Slice is not an identifier but a scoped name. You think it's ok to use lang:identifier in Ice Slice on modules where the argument of this "identifier" metadata is not an identifier? With your example: // module M is mapped to namespace MyWidget.M
["cs:identifier:MyWidget.M"] // MyWidget.M - the argument of cs:identifier - is not an identifier.
module M {} |
Interestingly, cppreference names ns-name "identifier": probably because it was an identifier before C++17. |
Your point isn't lost on me. But... yes, I do think it's okay. Although I agree it's not great. But think mechanistically. With your idea, we'd have two metadata: And they would have the following effect:
when compiled with
These effects are indistinguishable. In both cases, instead of using the Slice-provided identifier, a different name was generated based on some metadata. So, these sure feel like the same metadata to me. Just that it was artificially split into 2. |
My example is a little contrived though, because we're only ever dealing with identifiers. But okay. Nested module syntax looks like this:
An actual question: What would you call the Internally, in the Slice compiler, we call it a Line 382 in 50fbf7e
identifier , but it is still a kind of identifier.
And as you point out, the cppreference calls it an A similar argument applies to how you called it a "scoped name". Yes, Also, interestingly, cppreference has been using |
Regardless of which metadata we eventually select for modules, I recommend you rework this PR to focus exclusively on cpp:identifier for constructs other than modules. Metadata for modules / namespaces / scopes deserves its own PR. For now, just rename the Slice "keyword" modules in the C++ tests to something else. |
This PR implements #3264 and is a proof of concept for #2864 by adding support for this metadata to C++.
It does this by changing 3 fundamental functions in the
libSlice
Parser:name
: returns the identifier of a Slice definitionscope
: returns the enclosing scope of a Slice definition (with a trailing::
for the record)scoped
returns the fully-scoped identifier of a Slice definition (scope + name
)Currently on main, all 3 of these return the Slice identifier, and it's up the language mappings to massage them however.
Now, these functions take an optional
string_view langPrefix
parameter, allowing callers to signal that they want the identifier for a given mapping. When this parameter is present, these functions will check for<langPrefix>:identifier
metadata. If it is present, we return that metadata's argument. If not, we return the Slice identifier like normal.The general rule is:
One downside of this is that previously, is that it's more involved to compute an element's scope, because it's legal to put
xxx:identifier
on modules. So to resolve a scope for some Slice element, we have to check each of its parents for this metadata.