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

Add C++/winrt support for code generation #64

Closed
wants to merge 0 commits into from

Conversation

clarkezone
Copy link

@clarkezone clarkezone commented Feb 15, 2019

This PR adds support for c++/winrt code generation.

While the change works, it has a few todo's and will need to be addressed and also will need some refactoring as there are changes in instantiatorgeneratorbase that are c++/winrt specific which will need abstracting / specializing to avoid breaking c# generation.

It also disables c++/CX code generation (since the output filenames are the same for both) but does not remove the code. My assumption is that c++/winrt should replace c++/CX codegen, please confirm.

Appreciate an early review.

@dnfclas
Copy link

dnfclas commented Feb 15, 2019

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

❌ clarkezone sign now
You have signed the CLA already but the status is still pending? Let us recheck it.

@clarkezone clarkezone changed the title Ad C++/winrt support for code generation Add C++/winrt support for code generation Feb 15, 2019
Copy link
Contributor

@simeoncran simeoncran left a comment

Choose a reason for hiding this comment

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

We should keep side-by-side CX + WinrtCpp for now. For LottieViewer it only needs to support one and winrtcpp is the right thing for it to support when winrtcpp is working.
LottieGen has a switch to choose which one to output. In order to stop them overwriting each other can you add a suffix to the filename for CX, e.g. MyLottie_CX.cpp for CX and MyLottie.cpp for winrtcpp.

@@ -43,9 +43,9 @@ public override string CanvasGeometryCombine(Mgcg.CanvasGeometryCombine value)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than breaking this for Cx, can you just create a new version. I'd like CX to continue to work side by side.

@@ -55,7 +55,10 @@ async void SaveFile_Click(object sender, RoutedEventArgs e)

// Dropdown of file types the user can save the file as
filePicker.FileTypeChoices.Add("C#", new[] { ".cs" });
filePicker.FileTypeChoices.Add("C++ CX", new[] { ".cpp" });

// TODO: handle Cx generation side by side with CPPWINRT
Copy link
Contributor

Choose a reason for hiding this comment

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

Testing adding a comment from codeflow tool... #Closed

@sohchatt
Copy link
Contributor

@clarkezone Any updates?

@clarkezone
Copy link
Author

clarkezone commented Jun 18, 2019

Remaining todo-list for this PR

  • Fix hard coded changes in InstantiatorGeneratorBase to be target language neutral
  • Confirm that c++/winrt generation still works
  • Confirm that c# generation still works
  • Make c++/CX work again per code review feedback
  • Rebase / merge to Top of Tree

@clarkezone
Copy link
Author

LMK when you are ready to review this.. I only intend to rebase once and I only want to do that when you're ready to accept it, hence I'm going to stop work until I hear back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants