Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Codegen for module initializers feature #43281

Conversation

jnm2
Copy link
Contributor

@jnm2 jnm2 commented Apr 11, 2020

Test plan: #40500

I have simple codegen working and would like feedback on the approach.

The two things I know of that remain are marked by TODO comments:

I haven't started looking at either of them yet. Would this PR be good to merge to the feature branch this way?

@jnm2 jnm2 requested a review from a team as a code owner April 11, 2020 03:20
@jnm2
Copy link
Contributor Author

jnm2 commented Apr 11, 2020

I don't need to do anything with SynthesizedMetadataCompiler.ProcessSynthesizedMembers, right? In the emitMetadataOnly case, no synthesized root module type static constructor is created.

https://github.com/dotnet/roslyn/blob/db5c01e59a0fea97281a1adde921c097f48e73e6/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs#L2755-L2772

Answer: looks like we're going the no-synthesized-symbols route.

@RikkiGibson
Copy link
Contributor

Looks good to me so far, just have a bootstrap build failure that needs to be addressed.

@jnm2 jnm2 force-pushed the module_initializers/success_case branch from db5c01e to 46e61ef Compare April 12, 2020 15:36
@jnm2
Copy link
Contributor Author

jnm2 commented Apr 12, 2020

@RikkiGibson I forgot VB :-) (fix)

Copy link
Contributor

@RikkiGibson RikkiGibson left a comment

Choose a reason for hiding this comment

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

LGTM! Just suggest changing TODOs to the PROTOTYPE comment format we use in feature branches. This makes it a little easier to search and track down things we still need to do before merging the feature to master.

@RikkiGibson RikkiGibson requested a review from a team April 13, 2020 18:33
@RikkiGibson
Copy link
Contributor

@dotnet/roslyn-compiler could I please get a senior review on this?

@RikkiGibson RikkiGibson added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 13, 2020
@AlekseyTs
Copy link
Contributor

AlekseyTs commented Apr 24, 2020

I, personally, am not interested to see any intermediate wrong turns and typo fixes to be "burnt" into the history. We work on a specific, logically separate work item. Going through the process of trials and errors, etc. At the end we arrive to completion of the work item, that is what I would prefer us to record. I am not suggesting to squash PRs that integrate changes between branches, etc. Only PRs that are focused on specific work items, be it a bug fix or a logical step in a feature implementation. #Closed

@jnm2
Copy link
Contributor Author

jnm2 commented Apr 24, 2020

@AlekseyTs I think we view keeping the Git line history clean the same way. That's why I fix up commits for things like typos or anything else that is pure noise that doesn't document the changes. Depending on the intermediate wrong turn in question, I might or might not keep it around if I think it could potentially be interesting as future documentation. "This is what it looked like when we tried X." Anyway, there are no ideals, only tradeoffs, and what you say makes sense.

@jnm2 jnm2 requested a review from AlekseyTs April 24, 2020 22:37
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 22)

@AlekseyTs
Copy link
Contributor

@jnm2 If @RikkiGibson signs off, please let us know how do you want to go about merging this PR:

  • merge as is with 22 commits
  • you can do your own squashing re-basing ... + forced push
  • We can press "Squash and merge" button and all commits will be squashed together into a single commit by the GitHub.

@jnm2
Copy link
Contributor Author

jnm2 commented Apr 25, 2020

@AlekseyTs Squash merge is fine. Looking at the path these commits took, I'm not seeing anything of value that would be lost.

@AlekseyTs
Copy link
Contributor

Squash merge is fine.

Are we good to merge then or do you plan to make more changes in this PR?

@AlekseyTs
Copy link
Contributor

@dotnet/roslyn-infrastructure It looks like there is some kind of problem with roslyn-integration jobs

@jnm2
Copy link
Contributor Author

jnm2 commented Apr 26, 2020

@AlekseyTs I think it's ready to merge.

@jasonmalinowski
Copy link
Member

@AlekseyTs Merge the latest master into your feature branch; that should have all the fixes.

@AlekseyTs AlekseyTs merged commit 6fe4010 into dotnet:features/module-initializers Apr 28, 2020
@RikkiGibson
Copy link
Contributor

Thanks for your contribution @jnm2!

@jnm2 jnm2 deleted the module_initializers/success_case branch April 28, 2020 21:25
@jnm2
Copy link
Contributor Author

jnm2 commented Apr 28, 2020

@RikkiGibson @AlekseyTs Thanks very much both of you for all the help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants