[3.0] COM Binding Transformation #2406
Conversation
-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
|
@Perksey Ready for Review |
This reverts commit 0a1bd47.
| }, | ||
| "TransformCOM": { | ||
| "BaseTypes": { | ||
| "IUnknown.Interface": "Silk.NET.Windows.IUnknown.Interface" |
There was a problem hiding this comment.
To be honest, this is probably a good argument for putting IUnknown into Silk.NET.Core. The amount of stuff we had in Silk.NET.Core in 2.X was a bit ridiculous, but here I think it'd be justified as it's a fundamental ABI construct.
There was a problem hiding this comment.
Fair, but the initial thought for this option was for an api that was discussed a little while ago where it was com-like but had its own base class.
There was a problem hiding this comment.
Oh yeah absolutely, not saying get rid of the option, rather we should probably have unknwnbase.h (or whatever the header is) in Silk.NET.Core and use that for our default values for this option instead. IInspectable probably would make sense in Silk.NET.Core too.
There was a problem hiding this comment.
oh yeah, I can see that. I assume the thought here is for other COM libs being able to be pulled in without being reliant on the whole Silk.NET.Win32 package
|
|
||
| public override SyntaxNode? VisitVariableDeclaration(VariableDeclarationSyntax node) | ||
| { | ||
| if (node.Type.ToString() == "void**" && node.Variables.First().Identifier.ToString() == "lpVtbl") |
There was a problem hiding this comment.
What about the other variables? (If any ofc)
Comes back to my previous comment about maybe determining the pointer field from the type itself, afaik the only "is this COM" check we're doing is looking for the presence of a nested interface. We should probably check that the struct looks how we expect it to as well.
There was a problem hiding this comment.
Are you concerned about a case where a COM interface has an additional variable? because I don't think that ever happens as I believe that would violate it being COM. If we are thinking about a class that inherits from a COM class but isn't COM, I also don't think that happens, but this would be a problem.
We could check that only one variable is declared in our COM check and that it is only a void**, but again I believe for this case to come up would cause other issues with COM objects since they are pointers to shared objects afaik.
There was a problem hiding this comment.
as I believe that would violate it being COM
Exactly, so we should check :)
|
|
||
| var expression = node.Expression.ToString(); | ||
|
|
||
| if (expression != "Unsafe.AsPointer(ref this)") |
There was a problem hiding this comment.
This is of course assuming that we're within the struct methods, which we might not be (unless I've missed something). Is this something to worry about?
There was a problem hiding this comment.
We may potentially need to handle the case where this happens outside of an internal struct function (which wouldn't live in the rewriter but rather the usageUpdater) but so far it hasn't been an issue so wasn't one of the cases I've covered there.
| [return: NativeTypeName("ULONG")] | ||
| public uint AddRef() | ||
| { | ||
| return ((delegate* unmanaged<ID3DDestructionNotifier, uint>)((*lpVtbl)[1]))(this); |
There was a problem hiding this comment.
Do we have any Thiscall methods anywhere in the full bindings? I believe there may be an ABI difference between struct { void*** } and struct { void** }*
There was a problem hiding this comment.
I don't believe so but shouldn't those two be functionally the same? ptr->ptr->ptr
|
Obviously note the CI errors as well if you haven't already. |
-using globbing instead of regex -removed an unnecessary restriction on interface finding -updates single line code to expression bodies
| D3D11_CT_TBUFFER = D3D_CT_TBUFFER, | ||
|
|
||
| /// <include file='D3D_CBUFFER_TYPE.xml' path='doc/member[@name="D3D_CBUFFER_TYPE.D3D11_CT_INTERFACE_POINTERS"]/*'/> |
There was a problem hiding this comment.
Is it intentional that these are being generated again? (Assume we don't care for now and we're just suppressing the warning until we have a proper autodoc?)
(I know I asked for the commented out line to be something else other than a commented out line, but equally I assume this was done for a reason, but feel free to dismiss if this'll be dealt with later)
There was a problem hiding this comment.
Yeah it is intentional. I want to handle the autodoc later (possibly a separate PR) and for now it is just getting suppressed. I forgot I had the line commented out to check getting proper builds, but the intention is to have that there since even if we don't want these types of docs I do want the inheritdoc generated which is part of the same flag in clangsharp.
Co-authored-by: Dylan Perks <[email protected]>
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