[Mesa-dev] [v4 PATCH 05/10] mesa: helper function for scissor box of gl_framebuffer

[Ian Romanick]

On 05/27/2015 04:21 AM, Ilia Mirkin wrote:
> More trivia:
> 
> On Wed, May 27, 2015 at 5:49 AM, Kevin Rogovin <kevin.rogovin at intel.com> wrote:
>> Add helper convenience function that intersects the scissor values
>> against a passed bounding box. In addition, to avoid replicated code,
>> make the function _mesa_scissor_bounding_box() use this new function.
>>
>> v2:
>>  Split from patch "mesa:helper-conveniance functions for drivers to implement ARB_framebuffer_no_attachment".
>>  White space and long line fixes.
>>
>> v3:
>>  No changes.
>>
>> v4:
>>  No changes.
>>
>>
>> Signed-off-by: Kevin Rogovin <kevin.rogovin at intel.com>
>> ---
>>  src/mesa/main/framebuffer.c | 63 +++++++++++++++++++++++++++++++--------------
>>  src/mesa/main/framebuffer.h |  5 ++++
>>  2 files changed, 48 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
>> index c2cfb92..dd9e4bc 100644
>> --- a/src/mesa/main/framebuffer.c
>> +++ b/src/mesa/main/framebuffer.c
>> @@ -357,42 +357,38 @@ update_framebuffer_size(struct gl_context *ctx, struct gl_framebuffer *fb)
>>  }
>>
>>
>> +
>>  /**
>> - * Calculate the inclusive bounding box for the scissor of a specific viewport
>> + * Given a bounding box, intersect the bounding box with the scissor of
>> + * a specified vieport.
>>   *
>>   * \param ctx     GL context.
>> - * \param buffer  Framebuffer to be checked against
>>   * \param idx     Index of the desired viewport
>>   * \param bbox    Bounding box for the scissored viewport.  Stored as xmin,
>>   *                xmax, ymin, ymax.
>> - *
>> - * \warning This function assumes that the framebuffer dimensions are up to
>> - * date (e.g., update_framebuffer_size has been recently called on \c buffer).
>> - *
>> - * \sa _mesa_clip_to_region
>>   */
>> -void
>> -_mesa_scissor_bounding_box(const struct gl_context *ctx,
>> -                           const struct gl_framebuffer *buffer,
>> -                           unsigned idx, int *bbox)
>> +extern void
>> +_mesa_intersect_scissor_bounding_box(const struct gl_context *ctx,
>> +                                     unsigned idx, int *bbox)
>>  {
>> -   bbox[0] = 0;
>> -   bbox[2] = 0;
>> -   bbox[1] = buffer->Width;
>> -   bbox[3] = buffer->Height;
>> -
>>     if (ctx->Scissor.EnableFlags & (1u << idx)) {
>> +      int xmax, ymax;
>> +
>> +      xmax = ctx->Scissor.ScissorArray[idx].X
>> +        + ctx->Scissor.ScissorArray[idx].Width;
> 
> 44 instances of a leading + in mesa/main compared to 78 trailing ones.
> Huh, I was going to say that it's really uncommon to do this in mesa,
> but that may not be supported by fact.

I wrote a lot of the leading-operator cases.  I think most or all of
those are cases where the operator is in an if-condition.  Those cases
improve readability because of how the indentation lines up

    if (x +
        y)
        z;

vs.

    if (x
        + y)
        z;

It's more obvious that the second line is still part of the condition
when you're skimming.  Matt has made the argument that (and I'm
paraphrasing) like Yoda conditions, it may technically be better, but
it's weird... and weird is usually bad. *shrug*