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.
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
[Distributed] Implement distributed method accessors #40270
[Distributed] Implement distributed method accessors #40270
Changes from all commits
4860f90
b8358b2
9ebdf36
19bf160
ac2973d
a866a7a
185a663
2179db3
9d1c103
8980922
3ec9986
7190e9e
176b545
56fdf34
71e7355
2156fb0
38a0e66
8e1aa19
ef4e94e
a831017
0060859
be08a8b
3f4c06e
7eee8c0
d65906b
5af5501
7ac42e1
ed97120
111ceb1
dd0d5a8
151bae1
9b21742
304f3db
2180e4d
bc09fa3
8d14443
351300e
d6d9b55
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok... so this is good since it just says "name" and not exactly what shape of name it is (mangled, or just full name of the base etc). I remain a little worried worried about how we'll know what name to register methods under (today we do mangled names, but what if we want to do the other scheme -- we'd have to annotate methods I guess), but I guess it'll work out since we'd need annotations for the other scheme anyway perhaps 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might use flags to detect new scheme of naming if we switch to something other than mangled names.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My worry is about both "sides" having to use the same scheme so they'd have to be compiled using the same flags, but I guess that's just what we'll live with 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the unfortunate reality of the situation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh yeah... that's what I kept bringing up as "we change it later" being a problem... so we'll be forever stuck emitting the mangled type names as identifiers, and may be able to–in addition to those–add some shorter names... Unless we know "all sides" of the peers and can decide on a scheme to use with a compilation flag... I guess there's ways out of this corner but it'll be annoying :-/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think if we just wanted to drop all of the type information then it would be pretty easy to handle at runtime because we could just read mangled name from the record, remove the type information and match against the name provided by the caller. We'd only have to do that transformation once per method and cache it, but for other schemes it would be tricker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I wonder if we could get away with recording using the "full name" and then compare the mangled name if we have it to disambiguate any potential stores with the same name... 🤔
Not a blocker for merging this, but just something to continue thinking about :-)