[IMPL] Support IDxbcConverter from dxilconv.dll#417
[IMPL] Support IDxbcConverter from dxilconv.dll#417ds5678 wants to merge 3 commits intoterrafx:mainfrom
IDxbcConverter from dxilconv.dll#417Conversation
| @@ -0,0 +1,62 @@ | |||
| /////////////////////////////////////////////////////////////////////////////// | |||
There was a problem hiding this comment.
This file is notably in the "wrong" location, it should be somewhere under generation/include. It also, at minimum, requires updating the THIRD-PARTY-NOTICES.txt in the solution root and would be beneficial to know where it comes from.
That being said... typically the bindings in TerraFX.Interop.Windows are exclusive to what ships with Windows, the Windows SDK, or the DirectX Agility SDK. This header looks to be part of none of those and so is likely a "pit of failure" for users as it won't be usable "out of the box". The fact that it also creates a separate class to avoid conflicting with in-box APIs is problematic as well, because it creates a deviation from what users expect and where they're looking for APIs to be exposed.
This seems like something that should probably be its own repo/project, because its explicitly disconnected from what the DirectX SDK ships
There was a problem hiding this comment.
This file is notably in the "wrong" location, it should be somewhere under
generation/include. It also, at minimum, requires updating the THIRD-PARTY-NOTICES.txt in the solution root and would be beneficial to know where it comes from.
My apologies for these things. I can fix them.
That being said... typically the bindings in TerraFX.Interop.Windows are exclusive to what ships with Windows, the Windows SDK, or the DirectX Agility SDK. This header looks to be part of none of those and so is likely a "pit of failure" for users as it won't be usable "out of the box".
It should be usable "out of the box" since dxilconv.dll is in C:\Windows\System32.
The fact that it also creates a separate class to avoid conflicting with in-box APIs is problematic as well, because it creates a deviation from what users expect and where they're looking for APIs to be exposed.
The DxilConv class indeed has a discovery issue. That's why I asked about renaming.
This seems like something that should probably be its own repo/project, because its explicitly disconnected from what the DirectX SDK ships
So should I close my issue and pull request, or make changes to this pull request?
2f9287c to
f8efeb6
Compare
This implements support for
IDxbcConverterfromdxilconv.dllinC:\Windows\System32using the header from DirectXShaderCompiler.DxilConvso thatDxcCreateInstanceandDxcCreateInstance2wouldn't conflict with the ones inDirectXfordxcompiler.dll. Should I have renamed them toDxilConvCreateInstance/DxilConvCreateInstance2and put them in theDirectXclass?Resolves #416