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

Allow field layout to be determined for more types #267

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jack-pappas
Copy link
Contributor

Use the FormatterServices.GetUninitializedObject() method to get an instance of a specified type without having to invoke one of the ctors for the type; this allows the field layout to be determined for a wider set of types, e.g., types whose ctors check-then-throw for null arguments. An additional benefit of this change is that because the ctors for those types are no longer run, they no longer raise exceptions which cause the VS debugger to break (pause execution).

I built/ran the full test suite after making the change, and all of the tests passed except for one (an ISO8601-related test for TimeSpan) which doesn't seem related.

Use the FormatterServices.GetUninitializedObject() method to get an
instance of a specified type without having to invoke one of the ctors
for the type; this allows the field layout to be determined for a wider
set of types, e.g., types whose ctors check-then-throw for null
arguments. An additional benefit of this change is that because the
ctors for those types are no longer run, they no longer raise
exceptions which cause the VS debugger to break (pause execution).
@Wazner
Copy link
Contributor

Wazner commented Jun 18, 2017

If this pull request is approved, it will fix issue #207 . The failing unit test is no issue, I've found that some of the tests fail randomly because of a precision error.

As explained in the issue, GetUnitializedObject has a performance penalty of around 11%. I see you're always using it when it's available. It might benefit performance to only call GetUnitializedObject when no parameterless constructor is available.

@Timovzl
Copy link

Timovzl commented Mar 17, 2019

As explained in the issue, GetUnitializedObject has a performance penalty of around 11%. I see you're always using it when it's available. It might benefit performance to only call GetUnitializedObject when no parameterless constructor is available.

I strongly urge against this. In the spirit of correctness over performance, I suggest sticking to GetUninitializedObject(). Calling the constructor sometimes is more obscure, and could surprise the developer. More importantly, the behavior in question seems to be used even when serializing for the first time, if I remember correctly. When breakpoints are active, or there are other expectations on code [not] being hit, it is very disconcerting if a constructor is unexpectedly hit when serializing. When you serialize to string, you don't expect new instances of the classes in question to be constructed. And who knows - good practice or not, some constructors have side-effects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants