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

[Marek Olšák]

On Tue, May 24, 2016 at 10:16 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> 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 ?

OK, that sounds good. This patch is:

Reviewed-by: Marek Olšák <marek.olsak at amd.com>

Marek