-
Notifications
You must be signed in to change notification settings - Fork 86
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
Improve WinRT interop support #384
Conversation
…aler when a Win32 function has a WinRT object. Also added a new test project to demonstrate WinRT interop.
src/Microsoft.Windows.CsWin32/templates/WinRTCustomMarshaler.cs
Outdated
Show resolved
Hide resolved
var fromAbiMethod = type.GetMethod("FromAbi"); | ||
if (fromAbiMethod != null) | ||
{ | ||
return fromAbiMethod.Invoke(null, new object[] { pNativeData }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all seems incredibly expensive. Reflection, object allocations, etc. Could we perhaps cache it in a field once we've done it once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added caching to it.
{ | ||
using Stream? templateStream = Assembly.GetExecutingAssembly().GetManifestResourceStream($"{ThisAssembly.RootNamespace}.templates.{name.Replace('/', '.')}.cs"); | ||
if (templateStream is null) | ||
{ | ||
return string.Empty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems odd to me that we would return an empty string rather than null or throw if there is no name match. Are callers prepared to handle an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused by how nullable references work. I've changed this to return null instead.
// so that doesn't help. Looking at the name should be good enough, but if we needed to, the | ||
// Win32 projection could give us an attribute to make sure | ||
var objName = qualifiedName.Right.ToString(); | ||
bool isInterfaceName = System.Text.RegularExpressions.Regex.IsMatch(objName, "^I[A-Z][a-z]"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regular expression will match IFo
and IFo;ueu<
but not IFOo
. If you want any alphabet characters after an I
and no other characters, perhaps use ^I[A-Za-z]+$
as your expression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, to avoid reparsing/recompiling the regex repeatedly, I believe it's better to store a Regex
object in a static readonly field with the regex pattern, and use that from here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want a capital I, a capital letter, and a lower case character, and then I don't care what comes after. All WinRT interfaces will follow that convention. I did make the change to cache the regex.
test/WinRTInteropTest/stylecop.json
Outdated
@@ -0,0 +1,16 @@ | |||
{ | |||
"$schema": "https://raw.githubusercontent.com/DotNetAnalyzers/StyleCopAnalyzers/master/StyleCop.Analyzers/StyleCop.Analyzers/Settings/stylecop.schema.json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The repo already has this file. Do we really need another? If it was to set documentInternalElements: false
, you can just suppress the stylecop rule with an .editorconfig file in this directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird. I removed the stylecop.json and now I'm not seeing those warnings anymore. I'm not sure what was going on before.
….0 on build pipeline
…com/microsoft/CsWin32 into user/sotteson/WinRT-interop-support
In order to support the interop between Win32 and WinRT objects, I've added a custom marshaler that gets brought it when needed.
I also added a new test project to demonstrate WinRT interop.