Skip to content
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

Fix #486 - restore binary compat for inline resource API #497

Closed
wants to merge 5 commits into from

Conversation

latkin
Copy link
Contributor

@latkin latkin commented Jun 11, 2015

Internal error when consuming some F# 3.X libraries in F# 4.0 REPL, due to binary compat break.

Compat break was introduced in fix for loc issue #159. Restoring compat means this bug is now re-introduced.

  • Fix description
    • Restore old code in the resource API with the compat break
    • Do best-effort at keeping localization fix intact where possible
      • Loc issue is now restricted only to inline functions. The busted resource API was intended for use only by inline functions, but it had crept in to non-inline functions, as well. This is now corrected, so original localization issue is minimized as much as possible.
  • Testing done Cross-version compiler tests added, covering functions that use these resource APIs

@latkin
Copy link
Contributor Author

latkin commented Jun 12, 2015

ping @dsyme @enricosada to review. AFAICT switch to SR.GetString( ... ) in non-inline functions should be safe/correct and maintain localization fix where possible.

@dsyme
Copy link
Contributor

dsyme commented Jun 12, 2015

Yes, LGTM. Thanks to both of you for your diligence and efforts on this

@enricosada
Copy link
Contributor

good job @latkin , didn't think about other functions using ErrorStrings, LGTM :shipit:

@MattGertz
Copy link

MoreInfo 6/12 by ML Shiproom. @latkin Rollback for next release, full fix for go-ahead repo.

@MattGertz MattGertz assigned latkin and unassigned MattGertz Jun 12, 2015
@latkin
Copy link
Contributor Author

latkin commented Jun 12, 2015

Shiproom has requested that this be scoped purely to rollback of the binary compat issue. I've removed the partial loc fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants