Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Type.IsSZArray property.#16996

Merged
stephentoub merged 1 commit into
dotnet:masterfrom
JonHanna:fix_16566
Mar 14, 2017
Merged

Type.IsSZArray property.#16996
stephentoub merged 1 commit into
dotnet:masterfrom
JonHanna:fix_16566

Conversation

@JonHanna
Copy link
Copy Markdown
Contributor

Closes #16566

Won't pass CI until dotnet/coreclr#10105 and corert implementation (pending) are in.

yield return new object[] {typeof(int), false};
yield return new object[] {typeof(int[]).MakeByRefType(), false};
yield return new object[] {typeof(int[,]), false};
yield return new object[]{typeof(TypeTests), false };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Nit: missing space between "]" and "{"

yield return new object[] {Array.CreateInstance(typeof(int), new[] {2}, new[] {0}).GetType(), true};
yield return new object[] {typeof(int[][]), true};
yield return new object[] { Type.GetType("System.Int32[]"), true };
yield return new object[] { Type.GetType("System.Int32[*]"), false };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

typeof(int).MakeArrayType() and typeof(int).MakeArrayType(1) are a less roundable way to get these types than calling Type.GetType().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My intention had been to arive at types in a few different ways even if they're the same implementation underneath, so I've added the MakeArrayType() approach, but left these in.

yield return new object[] {Array.CreateInstance(typeof(int), new[] {2}, new[] {0}).GetType(), true};
yield return new object[] {typeof(int[][]), true};
yield return new object[] { Type.GetType("System.Int32[]"), true };
yield return new object[] { Type.GetType("System.Int32[*]"), false };
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

typeof(int).MakeArrayType() and typeof(int).MakeArrayType(1) are a less roundabout way to get these types than calling Type.GetType().

@stephentoub
Copy link
Copy Markdown
Member

@dotnet-bot test this please

@JonHanna
Copy link
Copy Markdown
Contributor Author

That netfx failure is definitely related. Do I need to do something to make it work there, or do something to make the test not run there?

@stephentoub
Copy link
Copy Markdown
Member

Do I need to do something to make it work there, or do something to make the test not run there?

I expect you need to guard the tests to only be compiled when targeting a build with the new surface area:
https://github.com/JonHanna/corefx/blob/998ec2696819fae352e8a03d08922bda741a5ecf/src/System.Runtime/tests/System.Runtime.Tests.csproj#L125

@JonHanna JonHanna force-pushed the fix_16566 branch 2 times, most recently from 7c00116 to 097a903 Compare March 13, 2017 22:12
arrType = enumBuilder.MakeArrayType(2);
Assert.True(arrType.IsArray);
Assert.False(arrType.IsSZArray);
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: in all of these tests, can you add blank lines between the relevant sets of tests, e.g.

Type asType = enumBuilder.AsType();
Assert.False(asType.IsArray);
Assert.False(asType.IsSZArray);

Type arrType = enumBuilder.MakeArrayType();
Assert.True(arrType.IsArray);
Assert.True(arrType.IsSZArray);

I find it hard to read when it's all smushed together like this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does suddenly look like a big block once you point that out! Changed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

😄

Copy link
Copy Markdown
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Thanks!

@stephentoub
Copy link
Copy Markdown
Member

@dotnet-bot test Outerloop Windows_NT Debug please

@stephentoub stephentoub merged commit 2ab05c3 into dotnet:master Mar 14, 2017
@JonHanna JonHanna deleted the fix_16566 branch March 14, 2017 16:37
@karelz karelz modified the milestone: 2.0.0 Mar 15, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants