-
Notifications
You must be signed in to change notification settings - Fork 831
Making several uses of Dictionary types be ConcurrentDictionary #10308
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
Conversation
cartermp
left a comment
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.
Looks great!
KevinRansom
left a comment
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.
Looks good, I do wonder, why a couple of tests changed the range, is it a quality of the dictionary type, or did something else subtle change.
...tOrientedTypeDefinitions/ClassTypes/MemberDeclarations/E_ClashingInstanceStaticProperties.fs
Show resolved
Hide resolved
...e/ObjectOrientedTypeDefinitions/ClassTypes/MemberDeclarations/E_PropertySameNameDiffArity.fs
Show resolved
Hide resolved
| ("Property", ["member"; "prop"])] | ||
| let results = [ for x in classTypeDefn.MembersFunctionsAndValues -> x.LogicalName, attribsOfSymbol x ] | ||
| [(".ctor", ["member"; "ctor"]); | ||
| ("get_Property", ["member"; "getter"]); |
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.
Elsewhere these are indented.
dsyme
left a comment
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.
Just a few changes requested
|
I added this issue: #10342 to manage the possible dictionary storage order dependency. |
dsyme
left a comment
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 looked again and no changes are needed
In order to make the compiler more concurrent safe, we needed to make several uses of
Dictionarytypes beConcurrentDictionarytypes. Remaining uses ofDictionarytypes have a comment noting why they are safe.