Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Remove test that sets static readonly field after type initialization #33413

Conversation

AndyAyersMS
Copy link
Member

dotnet/coreclr#20866 extends the jit to incorporate the type of readonly ref
class typed static fields into jitted code when jitting happens after class
initialization. For instance, the jit may optimize type tests or devirtualize
calls.

With the advent of type based jit optimizations we are also blocking reflection
based updates to all readonly static fields after class initialization.

dotnet/coreclr#20866 extends the jit to incorporate the type of readonly ref
class typed static fields into jitted code when jitting happens after class
initialization. For instance, the jit may optimize type tests or devirtualize
calls.

With the advent of type based jit optimizations we are also blocking reflection
based updates to all readonly static fields after class initialization.
@AndyAyersMS
Copy link
Member Author

cc @jkotas @VSadov @JonHanna

Func<PropertyAndFields> func = assignToReadonly.Compile(useInterpreter);
func();
Assert.Equal("ABC" + useInterpreter, PropertyAndFields.StaticReadonlyStringField);
}
Copy link
Member

Choose a reason for hiding this comment

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

I assume both the false and true variants of this test fail now (by design) with your change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately, no ... the compiled version will still "succeed."

Copy link
Member

Choose a reason for hiding this comment

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

There are number of other discrepancies between interpreted and compiled expressions if the expression is invalid. This is just one more.

Copy link
Contributor

Choose a reason for hiding this comment

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

They should be made consistent if there isn't a very good reason otherwise. I'll hopefully get back into contributing later this year, and I'd be interested in doing it then, but I'm by no means grabbing it if anyone else is interested.

@AndyAyersMS
Copy link
Member Author

CI showing some flaky failures. Any point in retrying them...?

@stephentoub
Copy link
Member

Any point in retrying them...

Since this is just deleting a test, no need. Thanks.

@stephentoub stephentoub merged commit 4914440 into dotnet:master Nov 12, 2018
@VSadov
Copy link
Member

VSadov commented Nov 12, 2018

LGTM

@AndyAyersMS AndyAyersMS deleted the WeAreAboutToDisallowSettingReadonlyStaticsAfterTypeInitialization branch November 12, 2018 18:32
@karelz karelz added this to the 3.0 milestone Nov 15, 2018
jlennox pushed a commit to jlennox/corefx that referenced this pull request Dec 16, 2018
…dotnet#33413)

dotnet/coreclr#20866 extends the jit to incorporate the type of readonly ref
class typed static fields into jitted code when jitting happens after class
initialization. For instance, the jit may optimize type tests or devirtualize
calls.

With the advent of type based jit optimizations we are also blocking reflection
based updates to all readonly static fields after class initialization.
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…dotnet/corefx#33413)

dotnet/coreclrdotnet/corefx#20866 extends the jit to incorporate the type of readonly ref
class typed static fields into jitted code when jitting happens after class
initialization. For instance, the jit may optimize type tests or devirtualize
calls.

With the advent of type based jit optimizations we are also blocking reflection
based updates to all readonly static fields after class initialization.

Commit migrated from dotnet/corefx@4914440
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants