Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Lookup System.Runtime and mscorlib references by Assembly name#10596

Merged
RussKeldorph merged 1 commit into
dotnet:masterfrom
mletterle:ilasm-sysruntime-alias
Apr 26, 2017
Merged

Lookup System.Runtime and mscorlib references by Assembly name#10596
RussKeldorph merged 1 commit into
dotnet:masterfrom
mletterle:ilasm-sysruntime-alias

Conversation

@mletterle
Copy link
Copy Markdown

Rather than by alias.

Fixes #10595

@dnfclas
Copy link
Copy Markdown

dnfclas commented Mar 30, 2017

@mletterle,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

@dnfclas
Copy link
Copy Markdown

dnfclas commented Mar 31, 2017

@mletterle, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

@dnfclas
Copy link
Copy Markdown

dnfclas commented Mar 31, 2017

@mletterle, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

@danmoseley
Copy link
Copy Markdown
Member

@dotnet/jit-contrib do you guys own ilasm?

@danmoseley
Copy link
Copy Markdown
Member

@dotnet-bot test OSX10.12 x64 Checked Build and Test (network)

@danmoseley
Copy link
Copy Markdown
Member

@dotnet-bot test Ubuntu x64 Checked Build and Test (log gone)

@danmoseley
Copy link
Copy Markdown
Member

@dotnet/jit-contrib could you please evaluate this.

@RussKeldorph
Copy link
Copy Markdown

@mletterle Apologies (again) for missing this PR. If you're happy with where #10878 is going, could you close this one? Thank you very much for helping make ilasm better. In the future, feel free to mention me directly on any ilasm/ildasm/jit-related issues or PRs so hopefully I don't miss them.

@mletterle
Copy link
Copy Markdown
Author

@RussKeldorph While the two PRs do solve the same problem, the code they contain is pretty orthogonal, my PR changes the way BaseAsmRef is determined which will affect how it is determined wherever that call is used. Which may fix other unreferenced issues. Though there is probably a discussion to be had on the whole "BaseAsmRef" idea in general.

Thanks for helping address these long standing issues! Exciting times in cil land...

@RussKeldorph RussKeldorph requested a review from rartemev April 20, 2017 18:13
@rartemev
Copy link
Copy Markdown

@dotnet-bot test Ubuntu x64 Checked Build and Test please

@rartemev
Copy link
Copy Markdown

@mletterle As I can see you changes solve the original issue but still cannot handle chained aliases like test from my PR does.

@mletterle
Copy link
Copy Markdown
Author

This PR is specifically to avoid aliases all together and look at the simple name of the Assembly to determine if it is System.Runtime for example.

Also I don't believe it's possible to define an alias that refers to another alias.

Partition II § 6.3 doesn't appear to provide a mechanism for that since it says:

The dotted name used in .assembly extern shall exactly match the name of the assembly as declared
with an .assembly directive
, in a case-sensitive manner

emphasis mine.

@rartemev
Copy link
Copy Markdown

@dotnet-bot test Ubuntu16.04 arm Cross Debug Build
@dotnet-bot test OSX10.12 x64 Checked Build and Test
@dotnet-bot test Ubuntu arm Cross Release Build

@rartemev
Copy link
Copy Markdown

Since all CI tests have passed it looks good to me. Besides that I am still not sure about alias chains so I'd rather ask @RussKeldorph to clarify this question.

@RussKeldorph RussKeldorph merged commit bf579f8 into dotnet:master Apr 26, 2017
@RussKeldorph
Copy link
Copy Markdown

@mletterle Thanks!

@mletterle mletterle deleted the ilasm-sysruntime-alias branch April 26, 2017 22:21
@karelz karelz modified the milestone: 2.0.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants