Multiple refactorings#168
Conversation
…esAndExtensionHeaders
|
@jkoritzinsky this PR is ready to be reviewed and merged. |
|
Reminder. |
jkoritzinsky
left a comment
jkoritzinsky
left a comment
There was a problem hiding this comment.
A few questions/comments here and there. Overall looks great!
|
|
||
| namespace SharpGen.Runtime | ||
| { | ||
| public static class ComActivationHelpers |
There was a problem hiding this comment.
All of the COM-specific types should live in SharpGen.Runtime.COM instead of in the SharpGen.Runtime base layer.
There was a problem hiding this comment.
I know, and yet I propose moving this class and the CLSCTX enum to SharpGen.Runtime, since it is independent of all other COM types, but provides bare minimum of the COM utilities. It is also used in tests in my mine branch. IUnknown and the rest will remain in SharpGen.Runtime.COM, along with the existing ComUtilities class, which will be reimplemented on top of this class.
| @@ -0,0 +1,25 @@ | |||
| <Project Sdk="Microsoft.NET.Sdk"> | |||
There was a problem hiding this comment.
What's the reasoning for splitting out SharpGen.Platform?
There was a problem hiding this comment.
To drop runtime-specific dependencies from SharpGen. RID-specific assembly lookup is highly error-prone (sometimes it just doesn't work, for example when MSBuild ALCs are used). This split removes RID-specific assemblies from SharpGen plugins' (e.g. SG.Doc.MSDN) dependency graph.
It also paves the road for the migration off the CastXML by creating interface and implementation parts. The interface is not a complete C++ parser abstraction, rather, it is a first draft of such.
| <TfmConsumerPropsLines Include="<Project>" /> | ||
| <TfmConsumerPropsLines Include="<ItemGroup>" /> | ||
| <TfmConsumerPropsLines Include='<SharpGenConsumerMapping Include="$(EmbeddedConsumerProp)(MSBuildThisFileDirectory)/$(SharpGenConsumerBindMappingConfigId).BindMapping.xml"/>' /> | ||
| <TfmConsumerPropsLines Include='<SharpGenConsumerMapping Include="%24(MSBuildThisFileDirectory)/$(SharpGenConsumerBindMappingConfigId).BindMapping.xml"/>' /> |
There was a problem hiding this comment.
Did you validate that the %24 works? I thought that that didn't work and you'd have to use something like the built-in MSBuild::Escape method.
There was a problem hiding this comment.
Yes, I've validated this to work.
| DependsOnTargets="CreateIntermediateDir;MapCppToCSharp;RemoveGeneratedCSharpFiles" | ||
| Inputs="@(SharpGenSdkAssembly);@(CSharpModel);@(DocLinksCache);@(SharpGenExternalDocs)" | ||
| Outputs="@(GeneratedCSharpFiles)" | ||
| Outputs="@(GeneratedCSharpFiles);$(SharpGenIntermediateDir)ForceNotUpToDate" |
There was a problem hiding this comment.
By adding the ForceNotUpToDate, this is basically removing the point of the Inputs and Outputs fields.
There was a problem hiding this comment.
Indeed. That was a mistake made when removing GetGeneratedCSharpFiles target. mine branch does everything differently.
|
@jkoritzinsky it would be great to finally have this merged. |
2 participants
See commit list for the list of changes, review by commit, instead of all at once.