-
Notifications
You must be signed in to change notification settings - Fork 5k
Add tests to check Memory<T>.Slice doesn't reset the high-bit in length #29251
Conversation
1b8c0ed
to
ca18db4
Compare
|
||
using System.Runtime.CompilerServices; | ||
|
||
[assembly: InternalsVisibleTo("System.Memory.Tests, PublicKey=002400000480000094000000060200000024000052534131000400000100010015c01ae1f50e8cc09ba9eac9147cf8fd9fce2cfe9f8dce4f7301c4132ca9fb50ce8cbf1df4dc18dd4d210e4345c744ecb3365ed327efdbc52603faa5e21daa11234c8c4a73e51f03bf192544581ebe107adee3a34928e39d04e524a9ce729d5090bfd7dad9d10c722c0def9ccc08ff0a03790e48bcd1f9b6c476063e1966a1c4")] |
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.
We've been trying very hard to avoid using InternalsVisibleTo. We've been stamping them out across the product assemblies where they've existed, and are resisting adding more.
cc: @weshaggard
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.
Instead of doing this, could we just Unsafe.As to reinterpret cast the Memory<T>
with a copy in the tests that has the same layout but that has public fields?
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.
(Note, too, that I believe InternalsVisibleTo will prevent the linker from trimming away dead code, including that GetObjectStartLength method added purely for testing purposes.)
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.
We've been trying very hard to avoid using InternalsVisibleTo.
Why are we doing this?
We still added InternalsVisibleTo in some places, like Pipelines.
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.
@stephentoub That's an interesting point re: the linker and something I don't think we'd previously considered. Thanks for the Unsafe.As
tip!
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.
Why are we doing this?
There are technical reasons, like preventing cruft in product assemblies we ship, allowing the linker to do its job, etc. There are meta reasons, like encouraging developers to test as best as possible via public API surface rather than taking the easy way out and testing internals directly, designing tests in a way that are overly coupled to implementation that makes them less portable across implementations, etc. And there are philosophical reasons, like wanting tests to best mirror code that customers can write. There will always be situations where a test really needs access to something to fully exercise things the way we want, and for those limited situations we can find workarounds like Unsafe.As, private reflection, etc., with it being a very conscious and deliberate act to take such a dependency, rather than accidentally using an internal because it happened to be exposed, etc.
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.
@stephentoub hit the major points, see https://github.com/dotnet/corefx/issues/1604 where I've stated similar opinions in the past.
…th (dotnet/corefx#29251) * Add tests to check Memory<T>.Slice doesn't reset the high-bit in length * Address PR feedback. * Fix spacing in test csproj Commit migrated from dotnet/corefx@2ccd0f1
Addendum to #29246
cc @stephentoub, @GrabYourPitchforks, @joshfree, @KrzysztofCwalina