- Notifications
You must be signed in to change notification settings - Fork830
Fix #492 - use ConvILTypeRefUnadjusted not ConvILTypeRef#510
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
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.
@dsyme there is now only one direct use ofConvILTypeRef, below on line 978. Can you take a second look and confirm that this call is correct?
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.
There would certainly be no harm in adjusting that one for uniformity, but its harmless either way - the case is only used for the built-in primitive types like these:https://github.com/Microsoft/visualfsharp/blob/fsharp4/src/fsharp/FSharp.Core/prim-types-prelude.fsi#L73 as mentioned herehttps://github.com/Microsoft/visualfsharp/blob/275b832e9dd1a4bd64ed3accd218384b901be1d2/src/fsharp/tast.fs#L3681. I think this specific case is probably only used for nativeptr if at all.
latkin commentedJun 19, 2015
This guy has also been approved |
Use correct API when encoding quoted assembly references.
ConvILTypeRefUnadjustedproperly handles the case where the reference is statically linked.ConvILTypeRefdoes not.These two used to be a single API. They were split in
640db00and it was a simple oversight that the wrong one was wired up here.We have private tests which cover these type providers, they were simply never run in a cross-version configuration. That's frustrating, as cross-version testing was automated recently withAutomated cross-targeting testing #446. Running those tests, the bug would have been found a month ago. With the fix, they all pass.