[PATCH v6 0/41] MR6207: Draft: include: Introduce new COM implementation helper macros.
I think it would be very nice to have something like that to reduce the burden of implementing COM interfaces. This shows for instance on the windows.gaming.input module a 30% LoC reduction (from ~6800 to ~4700), even though the module already had a boilerplate helper macros already. The MR introduces macros to automatically implement each IUnknown method, as well as all of them at once. It also includes a helper to implement IInspectable methods at once, as well as macros to forward both interface methods to a base interface or an outer object. Last, it provides higher-level macros to implement a main interface and any number of sub interfaces and generate IUnknown, forwarding and vtables for all of them at once, with IInspectable support when needed. It uses widl to generate additional per-interface macros, for things like inheritance and vtable generation. The rest of the macros are otherwise shared in a Wine-specific header. The implementation is split to show individual macro being used, although they are later replaced by higher-level macros. The individual helpers are still useful in some corner cases where specific behavior needs to be implemented, or for aggregating classes. -- v6: widl: Generate return traces for COM classes. widl: Generate method traces for COM classes. widl: Generate COM class code for IClassFactory. windows.gaming.input: Generate the provider COM class. windows.gaming.input: Generate the vector COM classes. windows.gaming.input: Generate the async COM classes. windows.gaming.input: Generate the manager COM classes. windows.gaming.input: Generate the force feedback COM classes. windows.gaming.input: Generate the ramp effect COM classes. windows.gaming.input: Generate the periodic effect COM classes. windows.gaming.input: Generate the constant effect COM classes. windows.gaming.input: Generate the condition effect COM classes. (broken) windows.gaming.input: Generate the racing wheel COM classes. windows.gaming.input: Generate the gamepad COM classes. windows.gaming.input: rename controller to raw_controller widl: Generate initializers for COM classes. widl: Generate vtables for COM classes. widl: Generate impl unwrappers for COM classes. widl: Generate QueryInterface for COM classes. widl: generate some query interface helpers widl: Generate default IUnknown / IInspectable implementation. widl: Generate IUnkonwn and IInspectable forwarding for COM classes. widl: Generate impl_from helpers for COM classes. windows.gaming.input: Use the generated COM class structs. widl: Generate some structs for COM classes. widl: Parse a widl-specific impl attribute on structs. makedep: Generate some new impl.h headers from the IDLs. widl: Generate some new impl.h headers from the IDLs. windows.gaming.input: Use a separate interface for IAgileObject. widl: Introduce a new append_declspec helper. widl: Inline write_args into write_type_right. widl: Cleanup indentation and variables in write_type_right. widl: Remove now unnecessary write_callconv argument. widl: Introduce a new append_type_left helper. widl: Split write_type_left into a write_type_definition_left helper. widl: Cleanup indentation and variables in write_type_left. widl: Introduce a new write_record_type_definition helper. widl: Move some type name construction out of write_type_left. widl: Remove unnecessary recursion for TYPE_BITFIELD. widl: Introduce a new append_basic_type helper. widl: Wrap strappend parameters in a new struct strbuf. This merge request has too many patches to be relayed via email. Please visit the URL below to see the contents of the merge request. https://gitlab.winehq.org/wine/wine/-/merge_requests/6207
@jacek I have worked on a different approach, generating a header file that can be kept private with each module, like you suggested. This now requires however to define the classes for which to generate the code in the IDLs (see https://gitlab.winehq.org/wine/wine/-/blob/98f0b1ad484192922890b7a3cf3e4ce81...) rather than only in the implementation sources, which I find a little bit inconvenient -as it is separate from the rest of the code - but I think I managed to keep it acceptable. In "class" structs (identified with an `impl` WIDL-specific attribute), interface members (identified with their `_iface` suffix) are listed to know which interfaces the code needs to be generated for, and a couple of other fields can be defined with special semantics (such as `refcount`, `class_name` for WinRT IInspectable, or COM aggregation outer with `_outer` suffix). Additional fields without specific semantics can also be included there and will be emitted in the generated structure, although there are probably better kept in a separate structure. Each "class" struct will generate a corresponding struct in the generated `_impl.h` header, as well as a class function table containing a couple of necessary helpers and all the methods left to be implemented. For each interface of the class, this generates some code for its methods which handle the impl struct unwrapping and call into the class function table, as well the vtables for them, and initializer helper for the class. The idea is then to use the class struct as a member of "impl" structs, replacing all the iface members we currently have, greatly reducing the necessary boilerplace. If the "impl" struct doesn't need any extra field, it's also possible to make the "impl" and the "class" structs be the same, which is useful for static factories for instance. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_107562
I generally like it more, thanks. I think it’s more flexible and may solve more problems than the previous approach. However, I’m less convinced about the details. It would be nice to make it more natural to use and avoid the `WIDL_impl_*` macros and the additional pseudo-vtbl (`*_funcs`) struct. Let me give an example of a possible solution. Suppose we have a few interfaces in a public `example.idl`: ``` import "unknwn.idl"; [ object, uuid(576cf264-8781-4cec-a313-c099f9574ea0) ] interface IMyInt : IUnknown { [propget] HRESULT value([out, retval] int *p); [propput] HRESULT value([in] int v); } [ object, uuid(4a8728a0-c2cf-4e92-ad3d-01368eaee830) ] interface IIncInt : IMyInt { HRESULT inc(int i); } [ object, uuid(4a8728a0-c2cf-4e92-ad3d-01368eaee830) ] interface IDecInt : IMyInt { HRESULT dec(int i); } ``` To implement it with widl’s help, the developer would create a new file `mydll_private.idl` (this could use your syntax, but I prefer making interfaces explicit rather than inferring them from `*_iface` names): ``` import "example.idl"; [ debug(mydll) // Make widl generate debug traces using mydll debug channel ] class myint { interface IIncInt; interface IDecInt; [default] interface IUnknown; // Generate IUnknown methods instead of leaving them to manual code int i; } ``` Then widl could generate two files: a header and a C source. The `mydll_private.h` file could look like this: ``` #include "example.h" struct myint { IIncInt IIncInt_iface; IDecInt IDecInt_iface; LONG ref; int i; }; HRESULT myint_QueryInterface(struct myint *This, REFIID riid, void **ppv); ULONG myint_AddRef(struct myint *This); ULONG myint_Release(struct myint *This); HRESULT myint_get_value(struct myint *This, int *p); HRESULT myint_put_value(struct myint *This, int v); HRESULT myint_inc(struct myint *This, int i); HRESULT myint_dec(struct myint *This, int i); void free_myint(struct myint *This); void init_myint(struct myint *This); ``` That’s just the declarations for what we’ll need. The `mydll_private.c` file could then contain all the generated definitions: ``` #include "mydll_private.h" DECLARE_DEBUG_CHANNEL(mydll); // Implemented because requested by IDL, otherwise would be left for manual implementation HRESULT myint_QueryInterface(struct myint *This, REFIID riid, void **ppv) { if (IsEqualGUID(riid, &IID_IUnknown)) { TRACE_(mydll)("%p -> IID_IUnknown %p\n", This, ppv); *ppv = &This->IIncInt_iface; } else if (IsEqualGUID(riid, &IID_IMyInt)) { TRACE_(mydll)("%p -> IID_IMyInt %p\n", This, ppv); *ppv = &This->IIncInt_iface; } else if (IsEqualGUID(riid, &IID_IIncInt)) { TRACE_(mydll)("%p -> IID_IIncInt %p\n", This, ppv); *ppv = &This->IIncInt_iface; } else if (IsEqualGUID(riid, &IID_IDecInt)) { TRACE_(mydll)("%p -> IID_IDecInt %p\n", This, ppv); *ppv = &This->IDecInt_iface; } else { TRACE_(mydll)("%p -> riid %s ppv %p\n", This, debugstr_guid(riid), ppv); *ppv = NULL; return E_NOINTERFACE; } myint_AddRef(This); return S_OK; } ULONG myint_AddRef(struct myint *This) { LONG ref = InterlockedIncrement(&This->ref); TRACE_(mydll)("%p ref=%ld\n", This, ref); return ref; } ULONG myint_Release(struct myint *This) { LONG ref = InterlockedDecrement(&This->ref); TRACE_(mydll)("%p ref=%ld\n", This, ref); if (!ref) free_myint(This); return ref; } static inline struct myint *myint_from_IIncInt(IIncInt *iface) { return CONTAINING_RECORD(iface, struct myint, IIncInt_iface); } static HRESULT WINAPI myint_IIncInt_QueryInterface(IIncInt *iface, REFIID riid, void **ppv) { struct myint *This = myint_from_IIncInt(iface); return myint_QueryInterface(This, riid, ppv); } static ULONG WINAPI myint_IIncInt_AddRef(IIncInt *iface) { struct myint *This = myint_from_IIncInt(iface); return myint_AddRef(This); } static ULONG WINAPI myint_IIncInt_Release(IIncInt *iface) { struct myint *This = myint_from_IIncInt(iface); return myint_Release(This); } static HRESULT WINAPI myint_IIncInt_get_value(IIncInt *iface, int *p) { struct myint *This = myint_from_IIncInt(iface); TRACE_(mydll)("%p -> p=%p\n", This, p); return myint_get_value(This, p); } static HRESULT WINAPI myint_IIncInt_put_value(IIncInt *iface, int v) { struct myint *This = myint_from_IIncInt(iface); TRACE_(mydll)("%p -> v=%d\n", This, v); return myint_put_value(This, v); } static HRESULT WINAPI myint_IIncInt_inc(IIncInt *iface, int i) { struct myint *This = myint_from_IIncInt(iface); TRACE_(mydll)("%p -> i=%d\n", This, i); return myint_inc(This, i); } static const IIncIntVtbl myint_IIncIntVtbl = { myint_IIncInt_QueryInterface, myint_IIncInt_AddRef, myint_IIncInt_Release, myint_IIncInt_get_value, myint_IIncInt_put_value, myint_IIncInt_inc, }; static inline struct myint *myint_from_IDecInt(IDecInt *iface) { return CONTAINING_RECORD(iface, struct myint, IDecInt_iface); } static HRESULT WINAPI myint_IDecInt_QueryInterface(IDecInt *iface, REFIID riid, void **ppv) { struct myint *This = myint_from_IDecInt(iface); return myint_QueryInterface(This, riid, ppv); } static ULONG WINAPI myint_IDecInt_AddRef(IDecInt *iface) { struct myint *This = myint_from_IDecInt(iface); return myint_AddRef(This); } static ULONG WINAPI myint_IDecInt_Release(IDecInt *iface) { struct myint *This = myint_from_IDecInt(iface); return myint_Release(This); } static HRESULT WINAPI myint_IDecInt_get_value(IDecInt *iface, int *p) { struct myint *This = myint_from_IDecInt(iface); TRACE_(mydll)("%p -> p=%p\n", This, p); return myint_get_value(This, p); } static HRESULT WINAPI myint_IDecInt_put_value(IDecInt *iface, int v) { struct myint *This = myint_from_IDecInt(iface); TRACE_(mydll)("%p -> v=%d\n", This, v); return myint_put_value(This, v); } static HRESULT WINAPI myint_IDecInt_dec(IDecInt *iface, int i) { struct myint *This = myint_from_IDecInt(iface); TRACE_(mydll)("%p -> i=%d\n", This, i); return myint_dec(This, i); } static const IDecIntVtbl myint_IDecIntVtbl = { myint_IDecInt_QueryInterface, myint_IDecInt_AddRef, myint_IDecInt_Release, myint_IDecInt_get_value, myint_IDecInt_put_value, myint_IDecInt_dec, }; void init_myint(struct myint *This) { This->IIncInt_iface.lpVtbl = &myint_IIncIntVtbl; This->IDecInt_iface.lpVtbl = &myint_IDecIntVtbl; This->ref = 1; } ``` And finally, the actual manual part of the implementation would be down to: ``` #include "mydll_private.h" HRESULT myint_get_value(struct myint *myint, int *p) { *p = myint->i; return S_OK; } HRESULT myint_put_value(struct myint *myint, int v) { myint->i = v; return S_OK; } HRESULT myint_inc(struct myint *myint, int i) { myint->i += i; return S_OK; } HRESULT myint_dec(struct myint *myint, int i) { myint->i -= i; return S_OK; } void free_myint(struct myint *myint) { free(myint); } struct myint *create_myint(void) { struct myint *ret = malloc(sizeof(*ret)); init_myint(ret); ret->i = 0; return ret; } ``` How does that sound to you? It’s mostly meant to show how it could be structured, with the details, syntax, coding style, etc. set aside for now. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112450
Well sure, this is the kind of output you would expect to see and most often what is being done with similar tools, but I think it's not great for various reasons and I wanted to avoid these in particular. Here's a couple of things I remember I wanted to avoid: * Moving class structure declarations entirely in the IDL breaks many tools integration. For instance LSP or others would point you to the generated file whenever you want to find the definition of a class member, not to the IDL itself where it would need to be edited. It would also require WIDL to be able to parse many more headers for any type to be usable in class members. With my approach I kept only the strict minimum in the IDL, which wouldn't need to be changed so often and I think it is an important feature to not make development more painful. * Using IDL-like syntax to declare classes like this would require to add WIDL-specific parsing support for it (ie: more code, more maintenance). OTOH reusing the struct parsing like I did just works already. Similarly, listing interfaces like here works for the most basic usage but as soon as you want to support things like inner / outer interfaces you will need to add WIDL-specific attributes as well and using naming conventions instead, which we already use pretty much everywhere anyway, actually seems more natural to me. * Using forward declarations and separate `.h`/`.c` also adds more indirection, and increases the chances to get unused functions go unnoticed, or link errors instead of compilation errors. It also adds more compilation units to build. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112476
I don’t understand the indirection argument. What I suggested has very little indirection. The generated call would be easy to inspect and very explicit. Compared to your branch, it removes the unnecessary `*_func` struct indirection and the whole macro trickery. As for maintenance, I think that if we move forward with this, we’ll end up with a lot of code using it but still only a single generator. I’d much rather prioritize user experience over saving a bit of code in widl. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112478
Yeah I don't mean indirection in term of call but rather in the separate function definition + declaration and separate .h/.c which you may then end up browsing back and forth instead of a single .h with everything in it. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112479
Another issue I see with your approach is that it doesn't seem to allow using it for static classes. You would have to export all the vtables and assemble them in the client code too, whereas with mine we can have static initializer macros directly usable. Fwiw I chose to use WIDL_impl macros just because I wanted to declare all the classes in the same IDL while keeping the existing implemention separation. It's a minor detail and we could very well either declare classes in separate IDLs (like your example), or generate a .h for each class instead (but that would possibly make it more difficult for makedep, and for the user to connect .h and their IDL). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112541
I don't buy the logic about going back and forth. The draft is meant to be easy to follow. To understand the whole thing, you just need to find `myint_IIncIntVtbl`, check the generated implementation (which is nothing more than a direct call to the actual implementation), then go to that implementation and you’re done. With your proposal, you’d have to go to the vtbl, find the implementation, and then wonder about the `*_funcs` struct. From there, you’d go to its declaration, then to the implementation, find the `*_FUNCS_INIT` macro, check that macro to figure out the implementation name, and finally go there. I don’t see that as an improvement. In fact, I think that’s actual indirection, and it’s something we should avoid. I wasn’t suggesting separate IDLs each class. In fact, I think a single one would be enough for most DLLs, and in most cases we already have such an IDL for registration purposes. That said, I’m not attached to my draft at all. I’m open to other solutions, but I do hope we can find something cleaner than the current version of this MR. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112624
I don't buy the logic about going back and forth. The draft is meant to be easy to follow. To understand the whole thing, you just need to find `myint_IIncIntVtbl`, check the generated implementation (which is nothing more than a direct call to the actual implementation), then go to that implementation and you’re done.
This is just one of point, and not the most critical one. Then I'm also talking from the experience I have so far as soon as functions are exported and used across sources, it's *always* more steps to navigate than when functions are defined directly in the same source (or its headers).
With your proposal, you’d have to go to the vtbl, find the implementation, and then wonder about the `*_funcs` struct. From there, you’d go to its declaration, then to the implementation, find the `*_FUNCS_INIT` macro, check that macro to figure out the implementation name, and finally go there. I don’t see that as an improvement. In fact, I think that’s actual indirection, and it’s something we should avoid.
Sure, well, you can also consider that this is only showing how much boilerplate we can strip and get generated, but nothing forces you to use these macros if you prefer to declare the class function table yourself (similarly you can also use any of the generated function freely if you prefer or need to build some exotic class that differs from what we generate). The macro only has the advantage that it prevents you from missing a function, and you will get a compilation error (which are much better than link errors as LSP tools can display them directly while writing code). Also, although it is static in the current state, the class function table could be made dynamic (could even be an optional thing driven by some IDL keyword) quite easily, making it possible to have multiple flavors of one class with the same boilerplate code. You can also adjust only some of the functions, similarly to how we do it with some interfaces. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112669
Sure, well, you can also consider that this is only showing how much boilerplate we can strip and get generated
No, it doesn't strip anything. It introduces a boilerplate with no clear advantage. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112693
nothing forces you to use these macros if you prefer to declare the class function table yourself (similarly you can also use any of the generated function freely if you prefer or need to build some exotic class that differs from what we generate).
Ideally, I shouldn't need to care about vtbl at all, at least by default. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112694
with no clear advantage.
"The macro [...] has the advantage that it prevents you from missing a function, and you will get a compilation error [if you do]." We may disagree but this is a clear advantage to me. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112707
On Tue Aug 12 14:14:18 2025 +0000, Rémi Bernon wrote:
with no clear advantage. "The macro [...] has the advantage that it prevents you from missing a function, and you will get a compilation error [if you do]." We may disagree but this is a clear advantage to me. Yes, I don't think that consideration of compile error vs linker error should a deciding factor when making decisions about the architecture. At very least, it's not more important than unnecessary boilerplate in both generated and manual code and sub-optimal indirections.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112710
On Tue Aug 12 14:43:43 2025 +0000, Jacek Caban wrote:
Yes, I don't think that consideration of compile error vs linker error should a deciding factor when making decisions about the architecture. At very least, it's not more important than unnecessary boilerplate in both generated and manual code and sub-optimal indirections. I think it should, in this particular case, because it's all about developer QoL and compilation errors play very nicely with LSP tools, while link errors do not and are a pain to deal with, and IMO even worse than the current situation, as verbose as it is to write.
I'm happy to make adjustments and to improve the experience for different use cases than mine but I'm ofc primarily interested in making my life easier in the first place, and won't be interested in making it worse. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112715
On Tue Aug 12 15:26:44 2025 +0000, Rémi Bernon wrote:
I think it should, in this particular case, because it's all about developer QoL and compilation errors play very nicely with LSP tools, while link errors do not and are a pain to deal with, and IMO even worse than the current situation, as verbose as it is to write. I'm happy to make adjustments and to improve the experience for different use cases than mine but I'm ofc primarily interested in making my life easier in the first place, and won't be interested in making it worse. Regarding this issue, could we get the best of both worlds by using a generated vtbl rather than a compiled one, but #including it rather than linking to it?
I'm not interested in bikeshedding (if I do have an opinion, I also prefer generated code to macros—I think they're far easier to read and understand for a newcomer), but the idea occurred to me and may allay concerns. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112724
On Tue Aug 12 16:39:07 2025 +0000, Elizabeth Figura wrote:
Regarding this issue, could we get the best of both worlds by using a generated vtbl rather than a compiled one, but #including it rather than linking to it? I'm not interested in bikeshedding (if I do have an opinion, I also prefer generated code to macros—I think they're far easier to read and understand for a newcomer), but the idea occurred to me and may allay concerns. The MR now generates code that is #included, but there still is a macro, which we are arguing about, which lists all the functions which are expected to be implemented, in a "class vtable", so you can't miss one when using it.
There might be other ways than a macro, but its purpose is however that it allows compilation errors over link errors and I think it's an important criteria not to be discarded. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112725
On Tue Aug 12 17:10:20 2025 +0000, Rémi Bernon wrote:
The MR now generates code that is #included, but there still is a macro, which we are arguing about, which lists all the functions which are expected to be implemented, in a "class vtable", so you can't miss one when using it. There might be other ways than a macro, but its purpose is however that it allows compilation errors over link errors and I think it's an important criteria not to be discarded. What I'm saying is that we can use Jacek's proposal above, but instead of putting it in a .c file, it's put in .h included from the implementation, and then myint_dec() et al. are static. That would cause compiler errors rather than linker errors, as you desire, and would be better for the compiler anyway as they can be inlined. Am I missing something?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_112726
On Tue Aug 12 17:20:29 2025 +0000, Elizabeth Figura wrote:
What I'm saying is that we can use Jacek's proposal above, but instead of putting it in a .c file, it's put in .h included from the implementation, and then myint_dec() et al. are static. That would cause compiler errors rather than linker errors, as you desire, and would be better for the compiler anyway as they can be inlined. Am I missing something? The issue is that these kind of warnings/errors don't seem to be reported by clangd. I'm not completely sure where this comes from but I believe it might be a decision, for performance reasons, as https://github.com/clangd/clangd/issues/137 suggests.
As the functions would be declared and used in the header files only, it would require the tools to fully analyze every header to catch every possible errors. For performance reasons the LSP tools don't perform full compilation, but take shortcuts to provide shorter response time. This was actually the reason why I chose to use a function table, which doesn't suffer from this issue, and keeps the user experience on a similar level as it is right now with individual interface vtables. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_113698
The issue is that these kind of warnings/errors don't seem to be reported by clangd. I'm not completely sure where this comes from but I believe it might be a decision, for performance reasons, as https://github.com/clangd/clangd/issues/137 suggests.
Wait, which kinds of errors? If myint_dec() is declared and referenced by the header, but never defined in the implementation, surely Clang reports an error for this? Are we talking about some other kind of error? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_113784
On Mon Aug 25 17:06:51 2025 +0000, Elizabeth Figura wrote:
The issue is that these kind of warnings/errors don't seem to be reported by clangd. I'm not completely sure where this comes from but I believe it might be a decision, for performance reasons, as https://github.com/clangd/clangd/issues/137 suggests. Wait, which kinds of errors? If myint_dec() is declared and referenced by the header, but never defined in the implementation, surely Clang reports an error for this? Are we talking about some other kind of error? The compiler will emit an error when compiling the file, yes. But LSP tools (clangd, which are usually reporting compile-time errors interactively while editing code in your favorite editor) will not and you will only notice a missing function when actually building. This would be a regression for anybody using LSP in their editor compared to the current situation.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/6207#note_113786
participants (3)
-
Elizabeth Figura (＠zfigura) -
Jacek Caban (＠jacek)
-
Rémi Bernon