Re: [PATCH 1/4] wine-headers: add a winapifamily header
First, the 1/4 patch is really something you'll have to convince Alexandre of it's the right thing to do. The easiest way is probably to ask him about it on IRC. The amount of advice I can give on that one is limited, but it would probably help if you could show why Wine needs the winapifamily mechanism. The best way to do that may very well just be to show that applications need e.g. the WINAPI_FAMILY_DESKTOP_APP constant to compile against the Wine headers.
I will ask him on IRC. Yes well it would be more correct to have the header in the case where a program uses that ifdef So this would match the official instructions from MS on using different api's I could also imagine that you'll be asked to partition the
other Wine headers as well if we're going to have this mechanism in Wine. I.e., I don't think it makes a lot of sense to only do this for a couple of dxgi headers, but not for anything else. The prefix for the subject should be "include:". I'd move this patch to the end of the series.
Initially I would have only done the dx stuff because mingw-w64 shares those headers. Essentially they Jacek Caban pulls in the d3d11 headers and idls from wine in to mingw-w64 to update. I could update the mingw-w64 project with the changes but I didn't want to leave wine out of the loop. I do understand that all headers would need to be partitioned to support this eventually. I was hoping the start would be with directx
The comment is pretty superfluous.
I can take it out. In patch 2/4, you have unrelated style changes for e.g.
IDXGIDevice2::OfferResources() . I happen to like the new style better, but please don't do that. First add the new stuff, then send a patch to make the existing stuff consistent with dxgi.idl at the end of the series.
Sure I can do that Some general style stuff:
- I think "{" on the same line for enum/struct/interface declarations is ugly. - Use "DXGI_OUTDUPL_DESC *desc" instead of "DXGI_OUTDUPL_DESC* desc", etc. - There are a couple of parameter naming issues. e.g. "pDevice", "colorSpace", "pStats", "Data", etc. - I prefer the order of things to be macros/constants, enums, structs, interfaces, functions instead of having those interleaved through the header. - "FLOAT" -> "float", "LPCWSTR" -> "const WCHAR *" - "num_*" -> "*_count"
The prefix for the dxgi IDL patches should be "dxgi:", for the d3d11 one it should be "d3d11:".
Seems easy enough to follow :) On Fri, Feb 6, 2015 at 1:05 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
I had a look at this series, and have a couple of comments.
First, the 1/4 patch is really something you'll have to convince Alexandre of it's the right thing to do. The easiest way is probably to ask him about it on IRC. The amount of advice I can give on that one is limited, but it would probably help if you could show why Wine needs the winapifamily mechanism. The best way to do that may very well just be to show that applications need e.g. the WINAPI_FAMILY_DESKTOP_APP constant to compile against the Wine headers. I could also imagine that you'll be asked to partition the other Wine headers as well if we're going to have this mechanism in Wine. I.e., I don't think it makes a lot of sense to only do this for a couple of dxgi headers, but not for anything else. The prefix for the subject should be "include:". I'd move this patch to the end of the series.
+/* wine wants all the api's by default so we set to desktop + app */ +#ifndef WINAPI_FAMILY +#define WINAPI_FAMILY WINAPI_FAMILY_DESKTOP_APP +#endif The comment is pretty superfluous.
In patch 2/4, you have unrelated style changes for e.g. IDXGIDevice2::OfferResources(). I happen to like the new style better, but please don't do that. First add the new stuff, then send a patch to make the existing stuff consistent with dxgi.idl at the end of the series.
Some general style stuff: - I think "{" on the same line for enum/struct/interface declarations is ugly. - Use "DXGI_OUTDUPL_DESC *desc" instead of "DXGI_OUTDUPL_DESC* desc", etc. - There are a couple of parameter naming issues. e.g. "pDevice", "colorSpace", "pStats", "Data", etc. - I prefer the order of things to be macros/constants, enums, structs, interfaces, functions instead of having those interleaved through the header. - "FLOAT" -> "float", "LPCWSTR" -> "const WCHAR *" - "num_*" -> "*_count"
The prefix for the dxgi IDL patches should be "dxgi:", for the d3d11 one it should be "d3d11:".
This is related to the other patch I sent in 108880 Superseded Martell Malone Re: Add d3d11_CreateDeviceAndSwapChain I never sent in a new patch for that. So I'm not sure how it was marked as Superseded On Sat, Feb 7, 2015 at 12:02 AM, Martell Malone <martellmalone(a)gmail.com> wrote:
First, the 1/4 patch is really something you'll have to convince
Alexandre of it's the right thing to do. The easiest way is probably to ask him about it on IRC. The amount of advice I can give on that one is limited, but it would probably help if you could show why Wine needs the winapifamily mechanism. The best way to do that may very well just be to show that applications need e.g. the WINAPI_FAMILY_DESKTOP_APP constant to compile against the Wine headers.
I will ask him on IRC. Yes well it would be more correct to have the header in the case where a program uses that ifdef So this would match the official instructions from MS on using different api's
I could also imagine that you'll be asked to partition the
other Wine headers as well if we're going to have this mechanism in Wine. I.e., I don't think it makes a lot of sense to only do this for a couple of dxgi headers, but not for anything else. The prefix for the subject should be "include:". I'd move this patch to the end of the series.
Initially I would have only done the dx stuff because mingw-w64 shares those headers. Essentially they Jacek Caban pulls in the d3d11 headers and idls from wine in to mingw-w64 to update.
I could update the mingw-w64 project with the changes but I didn't want to leave wine out of the loop. I do understand that all headers would need to be partitioned to support this eventually. I was hoping the start would be with directx
The comment is pretty superfluous.
I can take it out.
In patch 2/4, you have unrelated style changes for e.g.
IDXGIDevice2::OfferResources() . I happen to like the new style better, but please don't do that. First add the new stuff, then send a patch to make the existing stuff consistent with dxgi.idl at the end of the series.
Sure I can do that
Some general style stuff:
- I think "{" on the same line for enum/struct/interface declarations is ugly. - Use "DXGI_OUTDUPL_DESC *desc" instead of "DXGI_OUTDUPL_DESC* desc", etc. - There are a couple of parameter naming issues. e.g. "pDevice", "colorSpace", "pStats", "Data", etc. - I prefer the order of things to be macros/constants, enums, structs, interfaces, functions instead of having those interleaved through the header. - "FLOAT" -> "float", "LPCWSTR" -> "const WCHAR *" - "num_*" -> "*_count"
The prefix for the dxgi IDL patches should be "dxgi:", for the d3d11 one it should be "d3d11:".
Seems easy enough to follow :)
On Fri, Feb 6, 2015 at 1:05 PM, Henri Verbeet <hverbeet(a)gmail.com> wrote:
I had a look at this series, and have a couple of comments.
First, the 1/4 patch is really something you'll have to convince Alexandre of it's the right thing to do. The easiest way is probably to ask him about it on IRC. The amount of advice I can give on that one is limited, but it would probably help if you could show why Wine needs the winapifamily mechanism. The best way to do that may very well just be to show that applications need e.g. the WINAPI_FAMILY_DESKTOP_APP constant to compile against the Wine headers. I could also imagine that you'll be asked to partition the other Wine headers as well if we're going to have this mechanism in Wine. I.e., I don't think it makes a lot of sense to only do this for a couple of dxgi headers, but not for anything else. The prefix for the subject should be "include:". I'd move this patch to the end of the series.
+/* wine wants all the api's by default so we set to desktop + app */ +#ifndef WINAPI_FAMILY +#define WINAPI_FAMILY WINAPI_FAMILY_DESKTOP_APP +#endif The comment is pretty superfluous.
In patch 2/4, you have unrelated style changes for e.g. IDXGIDevice2::OfferResources(). I happen to like the new style better, but please don't do that. First add the new stuff, then send a patch to make the existing stuff consistent with dxgi.idl at the end of the series.
Some general style stuff: - I think "{" on the same line for enum/struct/interface declarations is ugly. - Use "DXGI_OUTDUPL_DESC *desc" instead of "DXGI_OUTDUPL_DESC* desc", etc. - There are a couple of parameter naming issues. e.g. "pDevice", "colorSpace", "pStats", "Data", etc. - I prefer the order of things to be macros/constants, enums, structs, interfaces, functions instead of having those interleaved through the header. - "FLOAT" -> "float", "LPCWSTR" -> "const WCHAR *" - "num_*" -> "*_count"
The prefix for the dxgi IDL patches should be "dxgi:", for the d3d11 one it should be "d3d11:".
Am 07.02.2015 um 01:10 schrieb Martell Malone <martellmalone(a)gmail.com>:
This is related to the other patch I sent in
108880 Superseded Martell Malone Re: Add d3d11_CreateDeviceAndSwapChain
I never sent in a new patch for that. So I'm not sure how it was marked as Superseded Probably by mistake, either because Alexandre saw my patches or your series as superseding this patch. Please resubmit it, but please make the following changes:
+HRESULT WINAPI D3D11CreateDeviceAndSwapChain(IDXGIAdapter *adapter, D3D_DRIVER_TYPE driver, HMODULE swrast, UINT flags, + const D3D_FEATURE_LEVEL *feature_levels, UINT levels, UINT sdk_version, Please adjust the indentation to 8 spaces here as well, as my patch has done for D3D11CreateDevice.
-cpp_quote("HRESULT WINAPI D3D11CreateDevice(IDXGIAdapter*,D3D_DRIVER_TYPE,HMODULE,UINT,const D3D_FEATURE_LEVEL*," ) +cpp_quote("HRESULT WINAPI D3D11CreateDevice(IDXGIAdapter*,D3D_DRIVER_TYPE,HMODULE,UINT,const D3D_FEATURE_LEVEL*,") I agree with this change, but technically it is unrelated to adding the stub, so please put it into a separate patch.
Hi Martell, On 02/07/15 01:02, Martell Malone wrote:
Initially I would have only done the dx stuff because mingw-w64 shares those headers. Essentially they Jacek Caban pulls in the d3d11 headers and idls from wine in to mingw-w64 to update.
I could update the mingw-w64 project with the changes but I didn't want to leave wine out of the loop. I do understand that all headers would need to be partitioned to support this eventually. I was hoping the start would be with directx
Yes, I reviewed them and patches seemed OK for me. FWIW, the amount of style comments that you received surprises me. We generally leave style of new files up to developer, unless it's really ugly, and I don't think it was. I warned you that style changes of existing code better avoided (patch 2), but you change arguments names at the same time, which makes it more acceptable. The real problem with your patches is something I didn't catch in the earlier review. I just tried to compile your code and got an error: make: Entering directory '/home/jacek/wine/wine-git/include' ../tools/widl/widl -o d3d11_1.h -m32 -I. -I../include -D__WINESRC__ d3d11_1.idl dxgi1_2.idl:383: error: syntax error, unexpected aIDENTIFIER Makefile:933: recipe for target 'd3d11_1.h' failed You use DXGI_MODE_DESC1, which is undeclared. I guess that you could tested headers in different environment, where it was declared, but please test things in Wine while sending patches. It's an easy way to validate your changes. In this case: tools/make_makefiles && make -C include/ is the very least to see the error. Obviously, full make afterwards is even better. As for winapifamily.h header, I don't see a problem with having it in Wine. Alexandre, if there is any reason that this particular patch is not accepted (other than being part of questionable series), please comment. Thanks, Jacek
make: Entering directory '/home/jacek/wine/wine-git/ include' ../tools/widl/widl -o d3d11_1.h -m32 -I. -I../include -D__WINESRC__ d3d11_1.idl dxgi1_2.idl:383: error: syntax error, unexpected aIDENTIFIER Makefile:933: recipe for target 'd3d11_1.h' failed
You use DXGI_MODE_DESC1, which is undeclared. I guess that you could tested headers in different environment, where it was declared, but please test things in Wine while sending patches. It's an easy way to validate your changes. In this case: tools/make_makefiles && make -C include/ is the very least to see the error. Obviously, full make afterwards is even better.
Yes I fixed this already I also had 2 variables with the same name and one or two other syntax errors but I haven't updated the patch as I was waiting for feedback. I will be doing a new series without any style changes or winapi family today. The process seems quite slow when I have loads of changes. So I decided to omit this. I'm actually using the headers to built SDL2 and OBS-studio so I know they are correct. Anyway I'll make the new series today :) On Sun, Feb 8, 2015 at 6:13 PM, Jacek Caban <jacek(a)codeweavers.com> wrote:
Hi Martell,
On 02/07/15 01:02, Martell Malone wrote:
Initially I would have only done the dx stuff because mingw-w64 shares those headers. Essentially they Jacek Caban pulls in the d3d11 headers and idls from wine in to mingw-w64 to update.
I could update the mingw-w64 project with the changes but I didn't want to leave wine out of the loop. I do understand that all headers would need to be partitioned to support this eventually. I was hoping the start would be with directx
Yes, I reviewed them and patches seemed OK for me. FWIW, the amount of style comments that you received surprises me. We generally leave style of new files up to developer, unless it's really ugly, and I don't think it was.
I warned you that style changes of existing code better avoided (patch 2), but you change arguments names at the same time, which makes it more acceptable.
The real problem with your patches is something I didn't catch in the earlier review. I just tried to compile your code and got an error:
make: Entering directory '/home/jacek/wine/wine-git/include' ../tools/widl/widl -o d3d11_1.h -m32 -I. -I../include -D__WINESRC__ d3d11_1.idl dxgi1_2.idl:383: error: syntax error, unexpected aIDENTIFIER Makefile:933: recipe for target 'd3d11_1.h' failed
You use DXGI_MODE_DESC1, which is undeclared. I guess that you could tested headers in different environment, where it was declared, but please test things in Wine while sending patches. It's an easy way to validate your changes. In this case: tools/make_makefiles && make -C include/ is the very least to see the error. Obviously, full make afterwards is even better.
As for winapifamily.h header, I don't see a problem with having it in Wine. Alexandre, if there is any reason that this particular patch is not accepted (other than being part of questionable series), please comment.
Thanks, Jacek
I sent in the new patchset. Total of 6 patches starting with CreateDeviceAndSwapChain as that was missed by the devs somehow. I also followed this where possible "FLOAT" -> "float", "LPCWSTR" -> "const WCHAR *" - "num_*" -> "*_count" Hopefully we can get this much merged On Mon, Feb 9, 2015 at 8:13 AM, Martell Malone <martellmalone(a)gmail.com> wrote:
make: Entering directory '/home/jacek/wine/wine-git/
include' ../tools/widl/widl -o d3d11_1.h -m32 -I. -I../include -D__WINESRC__ d3d11_1.idl dxgi1_2.idl:383: error: syntax error, unexpected aIDENTIFIER Makefile:933: recipe for target 'd3d11_1.h' failed
You use DXGI_MODE_DESC1, which is undeclared. I guess that you could tested headers in different environment, where it was declared, but please test things in Wine while sending patches. It's an easy way to validate your changes. In this case: tools/make_makefiles && make -C include/ is the very least to see the error. Obviously, full make afterwards is even better.
Yes I fixed this already I also had 2 variables with the same name and one or two other syntax errors but I haven't updated the patch as I was waiting for feedback.
I will be doing a new series without any style changes or winapi family today. The process seems quite slow when I have loads of changes. So I decided to omit this.
I'm actually using the headers to built SDL2 and OBS-studio so I know they are correct. Anyway I'll make the new series today :)
On Sun, Feb 8, 2015 at 6:13 PM, Jacek Caban <jacek(a)codeweavers.com> wrote:
Hi Martell,
On 02/07/15 01:02, Martell Malone wrote:
Initially I would have only done the dx stuff because mingw-w64 shares those headers. Essentially they Jacek Caban pulls in the d3d11 headers and idls from wine in to mingw-w64 to update.
I could update the mingw-w64 project with the changes but I didn't want to leave wine out of the loop. I do understand that all headers would need to be partitioned to support this eventually. I was hoping the start would be with directx
Yes, I reviewed them and patches seemed OK for me. FWIW, the amount of style comments that you received surprises me. We generally leave style of new files up to developer, unless it's really ugly, and I don't think it was.
I warned you that style changes of existing code better avoided (patch 2), but you change arguments names at the same time, which makes it more acceptable.
The real problem with your patches is something I didn't catch in the earlier review. I just tried to compile your code and got an error:
make: Entering directory '/home/jacek/wine/wine-git/include' ../tools/widl/widl -o d3d11_1.h -m32 -I. -I../include -D__WINESRC__ d3d11_1.idl dxgi1_2.idl:383: error: syntax error, unexpected aIDENTIFIER Makefile:933: recipe for target 'd3d11_1.h' failed
You use DXGI_MODE_DESC1, which is undeclared. I guess that you could tested headers in different environment, where it was declared, but please test things in Wine while sending patches. It's an easy way to validate your changes. In this case: tools/make_makefiles && make -C include/ is the very least to see the error. Obviously, full make afterwards is even better.
As for winapifamily.h header, I don't see a problem with having it in Wine. Alexandre, if there is any reason that this particular patch is not accepted (other than being part of questionable series), please comment.
Thanks, Jacek
participants (3)
-
Jacek Caban -
Martell Malone -
Stefan Dösinger