-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversation
929f0ed
to
5bb20f8
Compare
@dotnet/roslyn-compiler @agocke Please review the last commit. |
5bb20f8
to
0a86a90
Compare
@@ -111,6 +111,43 @@ internal static HashAlgorithm TryGetAlgorithm(AssemblyHashAlgorithm algorithmId) | |||
} | |||
} | |||
|
|||
internal static IDisposable TryGetAlgorithmImpl(AssemblyHashAlgorithm algorithmId) |
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.
Is there a more specific type that we can use?
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.
Please add a doc-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.
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.
Feedback addressed. Any more suggestions? |
|
||
default: | ||
// More algorithms can be added as needed. | ||
return null; |
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 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.
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.
Alternatively we should throw for other algorithms.
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.
It's not public API. It's private to the IncrementalHash implementation.
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'll change it to throw.
LGTM, modulo the issue with TryGetHashAlgorithmNameObj raised above. |
3bbd4cb
to
38db50e
Compare
Fixed. |
👍 |
#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