Reduce "unsafe" by replacing byte* pointer usage with ReadOnlySpan<byte> #3106
Conversation
ILSpy/Metadata/Helpers.cs
Outdated
| { | ||
| // endianess? |
There was a problem hiding this comment.
If I am not mistaken, the code below assumes little endian byte-order and ECMA335 states that big endian byte-order is only used in signatures. Maybe we should call this method "GetValueLittleEndian"?
There was a problem hiding this comment.
When decoding signatures, we should not need this method, as we already have access to the raw bytes and SRM BlobReader already exposes helpers that handle compressed integers.
There was a problem hiding this comment.
- renamed
GetValueLittleEndian - unless I'm misunderstanding these are not "compressed", they are just either "small" (2 byte) or "large" (4 bytes) depending on the size of the table they're indexing. Maybe this could be a
ReadTableIndexor something, but I'm not sure how that would work.
There was a problem hiding this comment.
I'm sorry, what is unclear? Which method on BlobReader could we use instead of GetValueLittleEndian?
There was a problem hiding this comment.
OK... now I understand... sorry.... apparently my second comment yesterday was the confusing one: It was just meant as additional information regarding LE vs BE encoding used in ECMA335. BE is only used in signature blobs, in which case we don't need GetValue (now called "GetValueLittleEndian"), because SRMs BlobReader already supports all the different primitive types used in signatures.
I did not mean to suggest using BlobReader instead of GetValue/GetValueLittleEndian. However, now that I think more about it, maybe we could/should use the methods found in BinaryPrimitives instead? Not sure whether that's a good idea for all the different uses of the method.
There was a problem hiding this comment.
You mean reimplementing GetValueLittleEndian in terms of BinaryPrimitives, or in each of the callers?
Also, it is public - do we have to go though a obsoletion/deprecation process or can it just be changed/removedv?
There was a problem hiding this comment.
I don't expect anyone to actually use the method outside of the decompiler engine or ILSpy, however, wouldn't do any harm to deprecate the original overload (the one that uses pointers) and keep the name "GetValue" so we don't break anyone, right?
I think all the uses of GetValue(LittleEndian) could be directly replaced by BinaryPrimitives?
|
Thank you for your contribution! |
This replaces all the
unsafecode involved in metadata parsing in the ILSpy project with the equivalent usingReadOnlySpan<byte>. There's a little bit left in ICSharpCode.Decompiler, mostly around resource handling.Not sure if it's worth it, especially given possible performance regression when running on netfx, but since I already did it as an experiment, it might be useful to the project.
side note: I think all ECMA 335 metadata is little endian, but the code doesn't really seem to deal with that. I don't think .net runs on any big endian machines anymore anyway.