On Tue, May 24, 2016 at 10:16 PM, Emil Velikov <[email protected]> wrote: > On 24 May 2016 at 17:38, Marek Olšák <[email protected]> wrote: >> On Tue, May 24, 2016 at 4:32 PM, Emil Velikov <[email protected]> >> wrote: >>> From: Emil Velikov <[email protected]> >>> >>> Using the macro to set the version is wrong and ill-advised. Please don't >>> do it. >>> >>> Cc: Marek Olšák <[email protected]> >>> Signed-off-by: Emil Velikov <[email protected]> >>> --- >>> Marek, now things start to unravel as to why you were not too excited on >>> the idea. Hopefully this comment makes things clearer/more descriptive. >>> If not please let me know how we can improve it. >>> --- >>> include/GL/mesa_glinterop.h | 9 ++++++--- >>> 1 file changed, 6 insertions(+), 3 deletions(-) >>> >>> diff --git a/include/GL/mesa_glinterop.h b/include/GL/mesa_glinterop.h >>> index 5c172c6..f637409 100644 >>> --- a/include/GL/mesa_glinterop.h >>> +++ b/include/GL/mesa_glinterop.h >>> @@ -93,7 +93,8 @@ enum { >>> * Device information returned by Mesa. >>> */ >>> typedef struct _mesa_glinterop_device_info { >>> - /* The caller should set this to: MESA_GLINTEROP_DEVICE_INFO_VERSION */ >>> + /* The caller should set this to the version of the struct they support >>> */ >>> + /* NOTE: Do not use the MESA_GLINTEROP_DEVICE_INFO_VERSION macro */ >>> uint32_t struct_version; >>> >>> /* PCI location */ >>> @@ -124,7 +125,8 @@ typedef struct _mesa_glinterop_device_info { >>> * Input parameters to Mesa interop export functions. >>> */ >>> typedef struct _mesa_glinterop_export_in { >>> - /* The caller should set this to: MESA_GLINTEROP_EXPORT_IN_VERSION */ >>> + /* The caller should set this to the version of the struct they support >>> */ >>> + /* NOTE: Do not use the MESA_GLINTEROP_EXPORT_IN_VERSION macro */ >>> uint32_t struct_version; >>> >>> /* One of the following: >>> @@ -183,7 +185,8 @@ typedef struct _mesa_glinterop_export_in { >>> * Outputs of Mesa interop export functions. >>> */ >>> typedef struct _mesa_glinterop_export_out { >>> - /* The caller should set this to: MESA_GLINTEROP_EXPORT_OUT_VERSION */ >>> + /* The caller should set this to the version of the struct they support >>> */ >>> + /* NOTE: Do not use the MESA_GLINTEROP_EXPORT_OUT_VERSION macro */ >>> uint32_t struct_version; >> >> Obviously, the comments don't apply to closed-source projects or any >> open source projects that just import the header and use the interface >> correctly and completely. If that's the case, using the VERSION macros >> makes sense. It had never been my intention for other projects to get >> this header from /usr/include/GL. Maybe we shouldn't install it. >> >> This patch would be OK if the header was used from /usr/include/GL >> only. Since that's not the expected usage, this patch is NAK. >> > I wasn't thinking/implying that the header should be installed... > maybe it should, maybe it shouldn't. The more important part is that > things are fragile as-is. > > For example: > User I needs v2 of export_in. Header is updated and implementation is done. > User A needs v2 of export_out. Header is updated and one will > - need to implement both v2 export_out _and_ v2 export_in (even if v2 > _in cannot be done for time/other reasons), or > - have to hack the header locally (a not good idea in itself), or > - don't implement v2 _in, in which case we'll crash. > > With ^^ in mind, I think it makes sense to advice against using the > macro, correct ?
OK, that sounds good. This patch is: Reviewed-by: Marek Olšák <[email protected]> Marek _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
