On 24 May 2016 at 17:38, Marek Olšák <mar...@gmail.com> wrote: > On Tue, May 24, 2016 at 4:32 PM, Emil Velikov <emil.l.veli...@gmail.com> > wrote: >> From: Emil Velikov <emil.veli...@collabora.com> >> >> Using the macro to set the version is wrong and ill-advised. Please don't >> do it. >> >> Cc: Marek Olšák <marek.ol...@amd.com> >> Signed-off-by: Emil Velikov <emil.veli...@collabora.com> >> --- >> 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 ? -Emil _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev