[Mesa-dev] [PATCH 03/14] mesa: add begin_transform_feedback() helper

[Timothy Arceri]

On 25/08/17 19:22, Samuel Pitoiset wrote:
> On 08/25/2017 02:36 AM, Timothy Arceri wrote:
>> On 24/08/17 23:21, Samuel Pitoiset wrote:
>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>>> ---
>>>   src/mesa/main/transformfeedback.c | 49 
>>> ++++++++++++++++++++++++---------------
>>>   1 file changed, 30 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/src/mesa/main/transformfeedback.c 
>>> b/src/mesa/main/transformfeedback.c
>>> index 307728c399..b217d0d84a 100644
>>> --- a/src/mesa/main/transformfeedback.c
>>> +++ b/src/mesa/main/transformfeedback.c
>>> @@ -381,22 +381,22 @@ get_xfb_source(struct gl_context *ctx)
>>>   }
>>> -void GLAPIENTRY
>>> -_mesa_BeginTransformFeedback(GLenum mode)
>>> +static ALWAYS_INLINE void
>>> +begin_transform_feedback(struct gl_context *ctx, GLenum mode, bool 
>>> no_error)
>>>   {
>>>      struct gl_transform_feedback_object *obj;
>>>      struct gl_transform_feedback_info *info = NULL;
>>> +   struct gl_program *source;
>>>      GLuint i;
>>>      unsigned vertices_per_prim;
>>> -   GET_CURRENT_CONTEXT(ctx);
>>>      obj = ctx->TransformFeedback.CurrentObject;
>>>      /* Figure out what pipeline stage is the source of data for 
>>> transform
>>>       * feedback.
>>>       */
>>> -   struct gl_program *source = get_xfb_source(ctx);
>>> -   if (source == NULL) {
>>> +   source = get_xfb_source(ctx);
>>
>>
>> Personally I would rather this left as is. We no longer have to bend 
>> to MCVSs refusal to support C99 for so many years.
> 
> It was for consistency actually, not for C99.

Sure, I get that but the reason it was done that way in the first place 
was because it needed to be to work with MSVC. IMO is just results code 
that is harder to follow and we are generally moving away from this 
style in newer code, it seems a bit backwards to be actively changing 
code back to this style.

> 
>>
>>
>>> +   if (!no_error && source == NULL) {
>>>         _mesa_error(ctx, GL_INVALID_OPERATION,
>>>                     "glBeginTransformFeedback(no program active)");
>>>         return;
>>> @@ -404,7 +404,7 @@ _mesa_BeginTransformFeedback(GLenum mode)
>>>      info = source->sh.LinkedTransformFeedback;
>>> -   if (info->NumOutputs == 0) {
>>> +   if (!no_error && info->NumOutputs == 0) {
>>>         _mesa_error(ctx, GL_INVALID_OPERATION,
>>>                     "glBeginTransformFeedback(no varyings to record)");
>>>         return;
>>> @@ -421,23 +421,26 @@ _mesa_BeginTransformFeedback(GLenum mode)
>>>         vertices_per_prim = 3;
>>>         break;
>>>      default:
>>> -      _mesa_error(ctx, GL_INVALID_ENUM, 
>>> "glBeginTransformFeedback(mode)");
>>> +      if (!no_error)
>>> +         _mesa_error(ctx, GL_INVALID_ENUM, 
>>> "glBeginTransformFeedback(mode)");
>>
>> You should be able to make this something like:
>>
>> if (!no_error) {
>>     _mesa_error(ctx, GL_INVALID_ENUM, "glBeginTransformFeedback(mode)");
>>     return;
>> } else {
>>     /* Stop compiler warnings */
>>     unreachable("Error in API use when using KHR_no_error");
>> }
> 
> Okay, looks good to me!
> 
> Thanks for the review.
> 
>>
>>
>>
>>>         return;
>>>      }
>>> -   if (obj->Active) {
>>> -      _mesa_error(ctx, GL_INVALID_OPERATION,
>>> -                  "glBeginTransformFeedback(already active)");
>>> -      return;
>>> -   }
>>> +   if (!no_error) {
>>> +      if (obj->Active) {
>>> +         _mesa_error(ctx, GL_INVALID_OPERATION,
>>> +                     "glBeginTransformFeedback(already active)");
>>> +         return;
>>> +      }
>>> -   for (i = 0; i < ctx->Const.MaxTransformFeedbackBuffers; i++) {
>>> -      if ((info->ActiveBuffers >> i) & 1) {
>>> -         if (obj->BufferNames[i] == 0) {
>>> -            _mesa_error(ctx, GL_INVALID_OPERATION,
>>> -                        "glBeginTransformFeedback(binding point %d 
>>> does not "
>>> -                        "have a buffer object bound)", i);
>>> -            return;
>>> +      for (i = 0; i < ctx->Const.MaxTransformFeedbackBuffers; i++) {
>>> +         if ((info->ActiveBuffers >> i) & 1) {
>>> +            if (obj->BufferNames[i] == 0) {
>>> +               _mesa_error(ctx, GL_INVALID_OPERATION,
>>> +                           "glBeginTransformFeedback(binding point 
>>> %d does not "
>>> +                           "have a buffer object bound)", i);
>>> +               return;
>>> +            }
>>>            }
>>>         }
>>>      }
>>> @@ -472,6 +475,14 @@ _mesa_BeginTransformFeedback(GLenum mode)
>>>   }
>>> +void GLAPIENTRY
>>> +_mesa_BeginTransformFeedback(GLenum mode)
>>> +{
>>> +   GET_CURRENT_CONTEXT(ctx);
>>> +   begin_transform_feedback(ctx, mode, false);
>>> +}
>>> +
>>> +
>>>   void GLAPIENTRY
>>>   _mesa_EndTransformFeedback(void)
>>>   {
>>>