Skip to content

Conversation

@333fred
Copy link
Member

@333fred 333fred commented Jan 31, 2020

@AlekseyTs @dotnet/roslyn-compiler for review.

@333fred 333fred requested review from a team and AlekseyTs January 31, 2020 19:49
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Feb 3, 2020

@333fred It looks like tests are failing #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Feb 3, 2020

internal struct TupleTypeDecoder

It feels like other decoders should also be adjusted: nullable, dynamic, etc. #Closed


Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/TupleTypeDecoder.cs:63 in c83dd64. [](commit_id = c83dd642d5f722a90583a2058438a16a8fc562ea, deletion_comment = False)

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2020

Choose a reason for hiding this comment

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

return CreateITypeSymbol [](start = 12, length = 24)

I think the code reuse doesn't give us a lot of benefits, but disallows us to assert that we properly use internal cash for public symbols. #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2020

Choose a reason for hiding this comment

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

return [](start = 12, length = 6)

Debug.Assert(nullableAnnotation != DefaultNullableAnnotation);? #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 3, 2020

Choose a reason for hiding this comment

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

ParamInfo retInfo = retAndParamTypes[0] [](start = 12, length = 51)

Why can't we do all this return type related work within the constructor? #Pending

Copy link
Member Author

Choose a reason for hiding this comment

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

We could. But given that Chuck just asked me to move what was in the constructor into a helper method, I'm going to leave this where it is.


In reply to: 374405675 [](ancestors = 374405675)

Copy link
Contributor

Choose a reason for hiding this comment

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

We could. But given that Chuck just asked me to move what was in the constructor into a helper method, I'm going to leave this where it is.

I am not sure how one is related to the other. I think, from the API design perspective, it would be better if constructor were taking all data in raw form, or all data in "preprocessed" form.


In reply to: 374952548 [](ancestors = 374952548,374405675)

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 4, 2020

Choose a reason for hiding this comment

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

CodeGenFunctionPointersTests [](start = 17, length = 28)

It might be useful to have targeted tests for metadata import from IL to ensure that things end up in the right places. #Closed

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Feb 4, 2020

Done with review pass (iteration 2) #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 4, 2020

Choose a reason for hiding this comment

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

SignatureCallingConvention [](start = 27, length = 26)

It feels like translating between two enums is just another hoop to jump over. Would it be possible to remove the Cci version of the enum and switch to using SignatureCallingConvention, which, I assume, is a public type and could be used in our public APIs as well? #Closed

@333fred
Copy link
Member Author

333fred commented Feb 4, 2020

internal struct TupleTypeDecoder

I'll put a prototype. I was specifically avoiding them in this pr.


In reply to: 581664963 [](ancestors = 581664963)


Refers to: src/Compilers/CSharp/Portable/Symbols/Metadata/PE/TupleTypeDecoder.cs:63 in c83dd64. [](commit_id = c83dd642d5f722a90583a2058438a16a8fc562ea, deletion_comment = False)

@333fred
Copy link
Member Author

333fred commented Feb 5, 2020

@AlekseyTs @cston addressed feedback.


In reply to: 581682378 [](ancestors = 581682378)

Copy link
Contributor

@cston cston Feb 5, 2020

Choose a reason for hiding this comment

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

convention & SignatureMask [](start = 35, length = 26)

Are we silently ignoring bits outside SignatureMask? Should we throw UnsupportedSignatureContent() when decoding those cases instead? #Pending

Copy link
Contributor

@cston cston Feb 5, 2020

Choose a reason for hiding this comment

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

Cci.CallingConventionUtils.FromSignatureConvention(signatureHeader.CallingConvention) [](start = 63, length = 85)

signatureHeader.CallingConvention.FromSignatureConvention() #WontFix

Copy link
Member Author

Choose a reason for hiding this comment

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

I specifically didn't do that here because we don't import Cci today, and I'm not willing to go through and remove the Cci. qualifiers from everything else in this file.


In reply to: 375063146 [](ancestors = 375063146)

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, there aren't that many here, I'll just do it.


In reply to: 375578610 [](ancestors = 375578610,375063146)

Copy link
Member Author

Choose a reason for hiding this comment

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

there aren't that many here

But there are conflicting names between Cci. and SRM., so I'll leave it.


In reply to: 375582556 [](ancestors = 375582556,375578610,375063146)

Copy link
Contributor

@cston cston Feb 5, 2020

Choose a reason for hiding this comment

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

Consider extracting a helper method for use here and in CreateFromMetadata() above. #Pending

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 5, 2020

Choose a reason for hiding this comment

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

MakeParametersFromMetadata [](start = 70, length = 26)

It looks like there is only one call site, consider making this a local function #Pending

Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (iteration 3), with refactoring suggestions.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Auto-approval

@ghost ghost merged commit 388d835 into dotnet:features/function-pointers Feb 6, 2020
@333fred 333fred deleted the emit-headers branch February 6, 2020 04:17
This pull request was closed.
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.

3 participants