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
Make config binding gen incremental #89587
Make config binding gen incremental #89587
Changes from 4 commits
4f6f853
0859bec
82318e7
ff62dd4
80c668f
321d65a
23abc0d
90be738
1b11a80
10f2579
de2e0e5
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.
Likewise, I believe the regex generator defines such a struct as well.
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.
At a glance this seems like a type that would be defined as a
record
if arrays were naturally equitable. Why wasn't this just defined as arecord
withImmutableEquatableArray<object?>
wrappingMessageArgs
?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.
It would force an intermediate allocation from
object[]
toImmutableEquatableArray
then back to anobject[]
when theDiagnostic
gets materialized. I think it's small enough to justify a bespoke type. Longer term though we should be looking at addressing dotnet/roslyn#68291 and useDiagnostic
in the equatable models directly.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.
Diagnostic
will never be ok to use in a model.Location
has a reference to theSyntaxTree
that it was created from, and that will change every time the file changes. This wrapper has similar issues: even if message args compared correctly, it would still be broken for equality.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.
Per #89587 (comment) this bit is mitigated for this PR, follow up in #92509.
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.
This provides a more array like semantic
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.
But it also makes it mutable?
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.
If the goal is immutable then make the return
readonly ref
.If the goal is immutalbe though then why are we inroducing
record
withset
members?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 think we should aim for the model being immutable everywhere.
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.
FWIW it seems
readonly ref
isn't supported on indexers yet.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.
Sorry, should be
ref readonly
notreadonly ref
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.
Did you consider just making the parameter type here
T[]
so that consumer can potentially avoid an allocation?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.
It makes a defensive copy to avoid encapsulating a potentially mutable source enumerable.
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.
Correct but if you make the parameter
T[]
there is no mutable source enumerable. The caller is forced to allocate the array and in the case the collection is already an array we don't double alloc.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.
Sure, but the array passed in by the caller could theoretically be mutated (e.g. it could be passing a rented buffer). Not super likely to happen here 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.
if this is all internal, i would make this just a T[]. You know your creators and can ensure they're doing the right thing.
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 these uint casts?
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.
consider:
Consider extracting out
_values.Length
to a fiel.