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

Conversation

@ghost
Copy link

@ghost ghost commented Sep 4, 2018

https://github.com/dotnet/corefx/issues/31614

  1. This will allow Reflection providers the option
    to supply the attribute type without building
    an entire constructor.

https://github.com/dotnet/corefx/issues/31798

  1. This will permit other Reflection providers
    to support Type.MakeGenericMethodParameter()
    in their implementations.

https://github.com/dotnet/corefx/issues/31614


1. This will allow Reflection providers the option
to supply the attribute type without building
an entire constructor.


https://github.com/dotnet/corefx/issues/31798

2. This will permit other Reflection providers
to support Type.MakeGenericMethodParameter()
in their implementations.
@ghost ghost self-assigned this Sep 4, 2018
@ghost
Copy link
Author

ghost commented Sep 4, 2018

cc @steveharter

@ghost ghost requested a review from jkotas September 4, 2018 14:28
throw new ArgumentNullException(nameof(typeArguments));
for (int i = 0; i < typeArguments.Length; i++)
{
if (typeArguments[i] == null)
Copy link
Member

Choose a reason for hiding this comment

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

It would be a bit more robust to validate this after cloning.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the validation here is intentionally minimal as the main use for this api is for MakeGenericType implementations which have already done the cloning.

Copy link
Member

@jkotas jkotas Sep 4, 2018

Choose a reason for hiding this comment

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

MakeGenericType implementations which have already done the cloning

Depends on the implementation. It is not required for MakeGenericType to clone the instantiation to call this. E.g. RtType.MakeGenericType does not clone the array before passing it to SignatureConstructedGenericType constructor.

What I meant is that the validation could be in SignatureConstructedGenericType constructor after the array is cloned.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm... that was an annoying oversight. Ok, replacing the debug.assert in the constructor with actual validation.

{
internal sealed class SignatureConstructedGenericType : SignatureType
{
internal SignatureConstructedGenericType(Type genericTypeDefinition, Type[] genericTypeArguments)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Rename this to typeArguments so that it has the same name as the public factory method? (The name shows up in the exception message.)

Copy link
Author

Choose a reason for hiding this comment

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

Ok, renamed.

@ghost ghost merged commit a1cb8f6 into dotnet:master Sep 4, 2018
@ghost ghost deleted the rol branch September 4, 2018 18:25
This pull request was closed.
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.

2 participants