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

Metadata writer refactoring (4F) #10833

Merged
merged 2 commits into from Apr 28, 2016
Merged

Metadata writer refactoring (4F) #10833

merged 2 commits into from Apr 28, 2016

Conversation

tmat
Copy link
Member

@tmat tmat commented Apr 25, 2016

#9699 ported to future.

Refactors PeWriter into components independent of Roslyn, so that it can later be moved to System.Reflection.Metadata in CoreFX.

The goal is to provide PE/COFF builder and ECMA-335 metadata builder that can be used broadly to write managed and native PE files. The goal is not to immediately provide the full support for writing all existing sections of the PE/COFF format but rather to enable composing PE/COFF files from section blobs and to provide builders for sections commonly appearing in managed PE files.

Includes implementation of a set of types, blob encoders that can be used for serialization of various blob structures found in the ECMA-335 metadata (especially signatures). The introduced abstraction provides type-system enforced structure to the blobs with minimal runtime overhead.

More work will follow to polish the public surface of the builders and serializers.

Tracked by work item: #6904

@tmat tmat changed the title Metadata writer refactoring Metadata writer refactoring (4F) Apr 25, 2016
@tmat
Copy link
Member Author

tmat commented Apr 26, 2016

@dotnet/roslyn-compiler @agocke Please review the last commit.

@@ -111,6 +111,43 @@ internal static HashAlgorithm TryGetAlgorithm(AssemblyHashAlgorithm algorithmId)
}
}

internal static IDisposable TryGetAlgorithmImpl(AssemblyHashAlgorithm algorithmId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a more specific type that we can use?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a doc-comment.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no more specific type we can use. I'll move this method to Incremental hash, since it's not really general purpose. Comment added.

@tmat
Copy link
Member Author

tmat commented Apr 27, 2016

Feedback addressed. Any more suggestions?


default:
// More algorithms can be added as needed.
return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not comfortable with returning null for all algorithms, but SHA1. What happens if other algorithm is requested? If we support only SHA1, then public API shouldn't pretend that this is not the case, by allowing to request different algorithm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively we should throw for other algorithms.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not public API. It's private to the IncrementalHash implementation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change it to throw.

@AlekseyTs
Copy link
Contributor

LGTM, modulo the issue with TryGetHashAlgorithmNameObj raised above.

@tmat
Copy link
Member Author

tmat commented Apr 27, 2016

Fixed.

@tmat
Copy link
Member Author

tmat commented Apr 27, 2016

@jaredpar @agocke Looks good?

@jaredpar
Copy link
Member

👍

@tmat tmat merged commit 710e0fc into dotnet:future Apr 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants