[3.0] COM Binding Transformation #2406

curin · 2024-12-31T14:58:36Z

Summary of the PR

Adds the necessary enhancements to allow for easy to use COM bindings and any changes required to get Microsoft Bindings building.

-Adds TransformCOM mod to implement in place ComPtr transformations in all COM classes
-Adds DisableWarnings mod to disable warnings in all files for a job in order to avoid build errors from unavoidable or ignorable errors
-Adds several additions to ClangScraper to disable caching and remove generated files from the output.
-Adds some QOL improvements such as --only and csv list support to silk touch

curin · 2025-01-10T02:25:21Z

@Perksey Ready for Review

Perksey

Nice work! It's cool to see this come together :)

Some notes...

sources/SilkTouch/SilkTouch/Clang/ClangScraper.cs

sources/SilkTouch/SilkTouch/Clang/ResponseFileHandler.cs

sources/SilkTouch/SilkTouch/Mods/DisableWarnings.cs

generator.json

sources/SilkTouch/SilkTouch/Mods/TransformCOM.cs

sources/Win32/Win32/DirectX/d3d12/d3dcommon/ID3DDestructionNotifier.gen.cs

Perksey · 2025-01-12T21:03:13Z

Obviously note the CI errors as well if you haven't already.

generator.json

sources/Win32/Win32/DirectX/d3d12/d3dcommon/D3D_CBUFFER_TYPE.gen.cs

sources/SilkTouch/SilkTouch/Mods/TransformInterfaces.cs

Perksey

Sorry this review is a bit all over the place, I wrote things as I saw them so comments may not be placed at their root causes.

Glad to see this moving along nicely.

Perksey · 2025-02-06T14:16:27Z

sources/Core/Core/DSL/MaybeBool`1.cs

+    /// <summary>
+    /// compares boolean value and int
+    /// </summary>
+    /// <param name="lh"></param>


I haven't really called this out in past PRs as it is a bit of a nitpick, but it would be nice if you did fill out the documentation template generated!

Perksey · 2025-02-06T14:17:51Z

sources/Core/Core/Pointers/Ptr.cs

+        [MethodImpl(
+            MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization
+        )]
+        public static implicit operator Ptr(nuint ptr) => new(ptr.ToPointer());


I'm not completely comfortable with -> Ptr being implicit. The other direction is fine, but we should make it a little harder to accidentally interpret a random integer as a pointer.

Perksey · 2025-02-06T14:18:23Z

sources/Core/Core/Pointers/Ptr.generic.cs

+        [MethodImpl(
+            MethodImplOptions.AggressiveInlining | MethodImplOptions.AggressiveOptimization
+        )]
+        public static implicit operator Ptr<T>(nuint ptr) => new((T*)ptr.ToPointer());


Same comment.

Perksey · 2025-02-06T14:19:33Z

sources/Core/Core/INativeInterface.cs

+    /// <summary>
+    /// Represents a native interface structure
+    /// </summary>
+    public unsafe interface INativeInterface


Is this required by ClangSharp? If not we probably want to hold off on designing generalised abstractions over OOP ABI mechanisms until we have information on the other mechanisms we want to support. Recommend folding into IComInterface for now.

Perksey · 2025-02-08T14:40:00Z

sources/SilkTouch/SilkTouch/Utility/ProgressBarUtility.cs

+    /// <summary>
+    /// Class that provides some simple functions for displaying a progress bar for the generator
+    /// </summary>
+    public static class ProgressBarUtility


Not a fan of this being static. I also don't like us interacting with stdout directly given that SilkTouch has been designed to be somewhat embeddable, in that you can create your own DI container and just call AddSilkTouch - it is possible that there is no desire in this application for SilkTouch to interact with the console at all.

If we're keeping this, I recommend making it a DI interface that is added to the service collection scoped to each job (this way we don't have to do the multi-threading handling either).

In the long run something similar to how MSBuild's TerminalLogger (the default one these days) logs to console would be cool (obviously with a progress bar) but there's no need to go too crazy.

This does not work well with multithreading as it is editing the console output directly so that the loading bar gets overwritten. It needs to be done with a certain statefulness across threads since the console exists across threads. Moving it to a central thread could work but unfortunately you need to handle the logger outputting to the console and need to remove the progress bar before writing logs.
This is meant as simple utility to be used in cases where my sanity was dwindling due to watching nothing happening on the console for minutes at a time.

I'm fine if we want to pull this out it was more for my sanity than anything and I was more like we'll it's here and works but to do it right would take more work.

Yeah I agree we should definitely figure something out to make the user experience better, but we should probably spend more time figuring out the best way.

Perksey · 2025-02-08T16:39:27Z

sources/SilkTouch/SilkTouch/Mods/TransformCOM.cs

+                    // Check if the type is a ComType
+                    if (typeInfoR.Type != null && ComTypes.Any(type => $"{type.Item1}*" == nameR))
+                    {
+                        return BinaryExpression(node.Kind(), node.Left, IdentifierName($"{node.Right.ToFullString()}.lpVtbl"));


Do we want to exclude fields that are named differently from TransformInterfaces? Not a leading question, genuinely not sure if we should keep the hardcoded check or whether it makes sense.

sources/SilkTouch/SilkTouch/Mods/TransformInterfaces.cs

Perksey · 2025-02-08T16:50:52Z

sources/SilkTouch/SilkTouch/Mods/TransformInterfaces.cs

+                FileScopedNamespaceDeclarationSyntax node
+            )
+            {
+                _namespace.Add(node.Name.ToString());


I think there's a common utility function that will get the namespace that a descendent node is in.

sources/SilkTouch/SilkTouch/Mods/TransformInterfaces.cs

Perksey · 2025-02-08T17:04:27Z

sources/SilkTouch/SilkTouch/Mods/TransformInterfaces.cs

+                                )
+                            )
+                            .WithSemicolonToken(Token(SyntaxKind.SemicolonToken))
+                            .WithLeadingTrivia(


Can we move some of the trivia construction out to something reusable? It's very verbose and repeated a lot throughout this file.

I do have this fixed as I handled all the casts to use the same function. Will let you know when that update is added

-also support for csv lists in --only and --skip

-Setup to run using new mod system properly -Added pointer member accessor to member accessor conversion -Added Logging

per Mod Performance

-Added early out -fix duplicate call -Add INativeGuid

-fixed some broken using from TerraFX files -Allowed BaseType to be specified for ComTypes -Added several files for override due to issue in generation

-allowed for multiple COM base types -allowed for addition of manual COM types -added several manual overrides -added all manual files necessary for compilation

Fixed some manual warnings as well

This reverts commit 0a1bd47.

-using globbing instead of regex -removed an unnecessary restriction on interface finding -updates single line code to expression bodies

Co-authored-by: Dylan Perks <[email protected]>

for rollback purposes

moved to internal Native struct INativeInterface and IComInterface added to represent all interfaces and Com interfaces, respectively. - IComInterface is a child of INativeInterface and INativeGuid

-and fixing issues with how it works

added manual file importer added a progress bar in some cases for QoL moved rename function into NameUtils

Added per job file logs fixed up the progress bar system to be cleaner

This reverts commit ed1c763.

Perksey changed the base branch from main to develop/3.0 December 31, 2024 15:04

curin marked this pull request as ready for review January 10, 2025 02:25

curin requested a review from a team as a code owner January 10, 2025 02:25

Perksey reviewed Jan 12, 2025

View reviewed changes

Perksey reviewed Jan 15, 2025

View reviewed changes

generator.json Outdated Show resolved Hide resolved

sources/Win32/Win32/DirectX/d3d12/d3dcommon/D3D_CBUFFER_TYPE.gen.cs Outdated Show resolved Hide resolved

curin commented Feb 8, 2025

View reviewed changes

sources/SilkTouch/SilkTouch/Mods/TransformInterfaces.cs Show resolved Hide resolved

Perksey requested changes Feb 8, 2025

View reviewed changes

Perksey and others added 21 commits February 15, 2025 18:33

Start work on documenting the API

ed1c763

Add --only commandline arg to silktouch

452bda9

-also support for csv lists in --only and --skip

Initial Work to Add COM transformations

cec591f

Add Win32 projects to the Solution

0e2c085

Added Logging to ClangScraper

5593b03

TransformCOM update

5f17c4e

-Setup to run using new mod system properly -Added pointer member accessor to member accessor conversion -Added Logging

Additional Perf Logging

4712536

per Mod Performance

More COM work

d28b57b

-Added early out -fix duplicate call -Add INativeGuid

Fix null assignments

8d81b80

-fixed some broken using from TerraFX files -Allowed BaseType to be specified for ComTypes -Added several files for override due to issue in generation

Finish Usage Update Fixes + misc updates

d23a01c

-allowed for multiple COM base types -allowed for addition of manual COM types -added several manual overrides -added all manual files necessary for compilation

Remove old generated Windows files

b0efaf9

Added casts + disabled blocking warnings

3d94412

Fixed some manual warnings as well

Disable Warnings, Remove Generated Files, and Cleanup

9f394e8

Generation Dump

f696f66

Revert "Generation Dump"

f41dc67

This reverts commit 0a1bd47.

D3DCommon Generation

358ff23

undo temp comment

bb8b897

Deduplication of code in DisableWarnings

623ecb0

Addressing Review Comments

2fa7ac8

-using globbing instead of regex -removed an unnecessary restriction on interface finding -updates single line code to expression bodies

Update generator.json

bb29617

Co-authored-by: Dylan Perks <[email protected]>

Pre TransformInterfaces Commit

0a5c7c5

for rollback purposes

curin added 8 commits February 15, 2025 18:39

TransformInterfaces TransformCOM split

b2cbb69

moved to internal Native struct INativeInterface and IComInterface added to represent all interfaces and Com interfaces, respectively. - IComInterface is a child of INativeInterface and INativeGuid

Adding TransformFunctions

af96ea8

-and fixing issues with how it works

Fixing Transform Functions

fcfb86c

added manual file importer added a progress bar in some cases for QoL moved rename function into NameUtils

Run CSharpier

3352032

Addressing some Review Feedback

1582ea0

cleanup from merging

54dd8dd

Logging and Progress Update

a806ddf

Added per job file logs fixed up the progress bar system to be cleaner

Revert Accidental addition of input changes

fe2f36c

This reverts commit ed1c763.

curin force-pushed the develop/3/com-bindings-refactor branch from 6c6d633 to fe2f36c Compare February 16, 2025 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[3.0] COM Binding Transformation #2406

[3.0] COM Binding Transformation #2406

curin commented Dec 31, 2024 •

edited

Loading

curin commented Jan 10, 2025

Perksey left a comment

Perksey commented Jan 12, 2025

Perksey left a comment

Perksey Feb 6, 2025

Perksey Feb 6, 2025

Perksey Feb 6, 2025

Perksey Feb 6, 2025

Perksey Feb 8, 2025

curin Feb 8, 2025

curin Feb 9, 2025

Perksey Feb 9, 2025

Perksey Feb 8, 2025

Perksey Feb 8, 2025

Perksey Feb 8, 2025

curin Feb 8, 2025 •

edited

Loading

[3.0] COM Binding Transformation #2406

Are you sure you want to change the base?

[3.0] COM Binding Transformation #2406

Conversation

curin commented Dec 31, 2024 • edited Loading

Summary of the PR

curin commented Jan 10, 2025

Perksey left a comment

Choose a reason for hiding this comment

Perksey commented Jan 12, 2025

Perksey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

curin Feb 8, 2025 • edited Loading

Choose a reason for hiding this comment

curin commented Dec 31, 2024 •

edited

Loading

curin Feb 8, 2025 •

edited

Loading