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

Remove suggestion to consider using Marshal.SizeOf. #33725

Merged
merged 1 commit into from
Feb 28, 2019

Conversation

eerhardt
Copy link
Member

@eerhardt eerhardt commented Feb 27, 2019

Marshal.SizeOf is not a direct replacement for sizeof.

Fix #26513

@eerhardt eerhardt requested a review from a team as a code owner February 27, 2019 20:55
@agocke
Copy link
Member

agocke commented Feb 27, 2019

Where in the docs is the difference between the two explained and how to choose which one?

@eerhardt
Copy link
Member Author

eerhardt commented Feb 27, 2019

https://docs.microsoft.com/en-us/dotnet/api/system.runtime.interopservices.marshal.sizeof?view=netframework-4.7.2#System_Runtime_InteropServices_Marshal_SizeOf__1

In the Remarks:

The size returned is the size of the unmanaged object. The unmanaged and managed sizes of an object can differ.

Unsafe.SizeOf unfortunately is relatively new, and the docs aren't as baked:

https://docs.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.unsafe.sizeof

In general, my understanding is:

  • Marshal.SizeOf - unmanaged size
  • Unsafe.SizeOf - managed size

A concrete example of this is Marshal.SizeOf<bool>() returns 4 where Unsafe.SizeOf<bool>() returns 1 on my Windows x64 machine on .NET Core 2.1.

I was also considering dropping the suggestion all together. And simply saying '{0}' does not have a predefined size, therefore sizeof can only be used in an unsafe context

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

If the APIs are too new to fully document, they're far too new to suggest in a compiler message.

Before adding this I think we need public documentation on how these APIs work, what's the difference between them, and when to choose one over the other.

@eerhardt
Copy link
Member Author

@agocke - would you approve of just dropping the suggestions altogether?

'{0}' does not have a predefined size, therefore sizeof can only be used in an unsafe context

@agocke
Copy link
Member

agocke commented Feb 27, 2019

We've had that message for years and I haven't seen a lot of complaints about it, so no.

@tannergooding
Copy link
Member

We've had that message for years and I haven't seen a lot of complaints about it, so no.

The message is generally misleading and is leading users towards a pit-of-failure because it impliesthat Marshal.SizeOf is a viable replacement for sizeof.

Just searching for CS0233 or sizeof vs Marshal.SizeOf comes up with a number of things ranging from issues logged on roslyn/csharplang, to questions on StackOverflow, to blog posts from people elaborating on hitting this issue.

If you are interested in the differences:
Marshal.SizeOf gives the size of the type that the marshaller would create when marshalling a type to native code. For example, Marshal.SizeOf<bool>() returns 4 and Marshal.SizeOf<char>() returns 1. But a struct containing a char or bool with the MarshalAs attribute might return a different value (such as 1). For example: struct TestBool2 { [MarshalAs(UnmanagedType.U1)] public bool x; } has a marshal size of 1 and [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] struct TestChar { public char c; } would have a marshal size of 2.

Unsafe.SizeOf<T> is just a workaround for the compilers limitation here: dotnet/csharplang#1828, it just does the sizeof IL instruction and always returns the managed size of the object. For example, Unsafe.SizeOf<bool> always returns 1 and Unsafe.SizeOf<char> always returns 2. From the runtime spec:

Returns the size, in bytes, of a type. typeTok can be a generic parameter, a reference type or a
value type.

For a reference type, the size returned is the size of a reference value of the corresponding type,
not the size of the data stored in objects referred to by a reference value.

[Rationale: The definition of a value type can change between the time the CIL is generated and
the time that it is loaded for execution. Thus, the size of the type is not always known when the
CIL is generated. The sizeof instruction allows CIL code to determine the size at runtime
without the need to call into the Framework class library. The computation can occur entirely at
runtime or at CIL-to-native-code compilation time. sizeof returns the total size that would be
occupied by each element in an array of this type – including any padding the implementation
chooses to add. Specifically, array elements lie sizeof bytes apart. end rationale]

@YairHalberstadt
Copy link
Contributor

If Unsafe.Sizeof literally calls sizeof I would imagine it's the correct error message.

However the message as it currently stands is pointing people towards an incorrect replacement, so the suggestion should almost certainly be removed IMO.

@agocke
Copy link
Member

agocke commented Feb 27, 2019

I did as @tannergooding suggested and looked at some of the Stack Overflow suggestions. It does seem like people can be confused about this. Given that, I'll reverse my position and support deleting it.

Marshal.SizeOf is not a direct replacement for `sizeof`.

Fix dotnet#26513
@eerhardt eerhardt changed the title Add suggestion to consider using Unsafe.SizeOf Remove suggestion to consider using Marshal.SizeOf. Feb 27, 2019
@eerhardt
Copy link
Member Author

Thanks for the discussion everyone.

I've changed it to remove the suggestions altogether. And I've updated the .xlf files to make CI happy.

Copy link
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

@agocke agocke requested a review from a team February 27, 2019 23:17
@tannergooding
Copy link
Member

CC. @tmeschter on the xlf changes.

@tmeschter
Copy link
Contributor

@tannergooding Right now I only care about .xlf changes if you plan on trying to get this into 16.0. If this is for 16.1 then I have no concerns.

@agocke agocke merged commit 9afd4e7 into dotnet:master Feb 28, 2019
@eerhardt eerhardt deleted the patch-1 branch August 28, 2019 19:32
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.

7 participants