Skip to content

Remove un-necessary tuplification code#25941

Merged
jcouv merged 1 commit intodotnet:masterfrom
jcouv:dead-code2
Apr 6, 2018
Merged

Remove un-necessary tuplification code#25941
jcouv merged 1 commit intodotnet:masterfrom
jcouv:dead-code2

Conversation

@jcouv
Copy link
Copy Markdown
Member

@jcouv jcouv commented Apr 4, 2018

When looking at this method in the nullable feature branch (which looks a bit different) I noticed this un-necessary tuplification.
The PEParameterSymbol.Create call below, which ultimately rests on the PEParameterSymbol, tuplifies in all cases (with names if we have a parameter handle and that handle has the attribute for tuple names).

I'm leaving the call to dynamify (AsDynamicIfNoPia, on line above), since I think it does something different than what PEParameterSymbol does.

@jcouv jcouv added this to the 15.8 milestone Apr 4, 2018
@jcouv jcouv self-assigned this Apr 4, 2018
@jcouv jcouv requested a review from a team as a code owner April 4, 2018 23:11
@jcouv jcouv changed the title Remove un-necessary code Remove un-necessary tuplification code Apr 4, 2018
Copy link
Copy Markdown
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 1)

@AlekseyTs
Copy link
Copy Markdown
Contributor

Please check VB implementation as well.

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Apr 5, 2018

VB implementation looks fine (LoadSignature in PEMethodSymbol.vb).
I suspect that since VB doesn't support NoPia, there was no pre-existing special case for dynamic there that could have misled us.

@AlekseyTs
Copy link
Copy Markdown
Contributor

since VB doesn't support NoPia

It does.

there was no pre-existing special case for dynamic

Correct, VB doesn't have a concept of dynamic.

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Apr 6, 2018

@dotnet/roslyn-compiler Please review (one-liner). Thanks

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Apr 6, 2018

test windows_release_unit32_prtest please

@jcouv
Copy link
Copy Markdown
Member Author

jcouv commented Apr 6, 2018

test windows_debug_spanish_unit32_prtest please

@jcouv jcouv merged commit b6880c9 into dotnet:master Apr 6, 2018
@jcouv jcouv deleted the dead-code2 branch April 6, 2018 21:49
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