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
[cdac] Read types from contract descriptor #101994
[cdac] Read types from contract descriptor #101994
Changes from 5 commits
6cd4a60
8c92d38
8ff13a9
b282aa9
0d6a4c2
c617a4e
fa4f669
1ad1da3
790a4f9
16830cd
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.
Why is this okay? Does the
contract
not really matter in this case? The below logic means we could fail to add thecontract
because one already exists, and then return the one we just created. This seems like we could get into a state where an implementation has state and isn't the one being used. I think we need some other invariant in the system if this pattern is considered safe.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.
Agreed, this should return the existing one if it got registered in between. I don't think the contracts should really have state other than what is tied to the target itself.
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.
I kind of want contract implementations to be valuetypes. That way it's evident they have no identity. Not sure if that's going overboard, though
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.
That is the kind of architecture invariant that would work for me. It isn't a compile time enforcement, but does make people ask the right question. The counter to that though then becomes, why do we have a cache if none of the objects have state? Will the constructor have a lot of logic to set itself up?
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 feeling is, yes. (well not literally the constructor, but the
Create
static virtual method). In the C++ prototype there was a lot of work upfront to query the target and establish that the necessary data was available. I think something similar is already evident in theThread
contract, tooThere 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.
1ad1da3 turns
Thread
into a readonly struct - thoughts? I didn't want the additional interface originally, but after actually doing it, it doesn't seem like too much.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.
I like it, but I will also readily acknowledge we landed in a very non-idiomatic way of programming C#. But maybe with static virtuals and DIMs this is what programming with immutable values looks like in C# now?