[Mesa-dev] [PATCH 09/14] mesa_glinterop: fix GL interop *_VERSION comments

[Emil Velikov]

On 24 May 2016 at 17:38, Marek Olšák <maraeo at gmail.com> wrote:
> On Tue, May 24, 2016 at 4:32 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> From: Emil Velikov <emil.velikov at collabora.com>
>>
>> Using the macro to set the version is wrong and ill-advised. Please don't
>> do it.
>>
>> Cc: Marek Olšák <marek.olsak at amd.com>
>> Signed-off-by: Emil Velikov <emil.velikov at 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