[Piglit] [PATCH 2/3] arb_texture_view: Test valid and invalid formats

[Ian Romanick]

On 10/17/2013 01:39 PM, Jon Ashburn wrote:
> 
> On 10/17/2013 12:46 PM, Ian Romanick wrote:
>> On 10/16/2013 04:37 PM, Jon Ashburn wrote:
>>> diff --git a/tests/spec/arb_texture_view/common.c
>>> b/tests/spec/arb_texture_view/common.c
>>> new file mode 100644
>>> index 0000000..c5f5a23
>>> --- /dev/null
>>> +++ b/tests/spec/arb_texture_view/common.c
>>> @@ -0,0 +1,48 @@
>>> +/*
>>> + * Copyright © 2013 LunarG, Inc.
>>> + *
>>> + * Permission is hereby granted, free of charge, to any person
>>> obtaining a
>>> + * copy of this software and associated documentation files (the
>>> "Software"),
>>> + * to deal in the Software without restriction, including without
>>> limitation
>>> + * the rights to use, copy, modify, merge, publish, distribute,
>>> sublicense,
>>> + * and/or sell copies of the Software, and to permit persons to whom
>>> the
>>> + * Software is furnished to do so, subject to the following conditions:
>>> + *
>>> + * The above copyright notice and this permission notice (including
>>> the next
>>> + * paragraph) shall be included in all copies or substantial
>>> portions of the
>>> + * Software.
>>> + *
>>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>>> EXPRESS OR
>>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>> MERCHANTABILITY,
>>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO
>>> EVENT SHALL
>>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES
>>> OR OTHER
>>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
>>> ARISING
>>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>>> OTHER DEALINGS
>>> + * IN THE SOFTWARE.
>>> + *
>>> + * Author: Jon Ashburn <jon at lunarg.com>
>>> + */
>>> +
>>> +#include "GL/gl.h"
>>> +#include <stdarg.h>
>>> +
>>> +
>>> +void update_valid_arrays(GLenum *valid, GLenum *invalid,
>>> +                         unsigned int numInvalid, unsigned int
>>> numValid, ... )
>> I the same review feedback where I suggested refactoring this function
>> to a common place, I also suggested changing the signature to:
>>
>> unsigned update_valid_arrays(GLenum *valid, GLenum *invalid,
>>                               unsigned int numInvalid, ... );
>>
>> Is there a reason you didn't also do that?
> Which common place are you referring to?  tests/util directory?  I just
> used
> shared file within the test directory for "common" place.

What you've done here is fine... and exactly what I meant.

> Why change the signature to return unsigned?  Error return value?

Ugh... I blame Thunderbird.  It apparently decided to not send that
message, which is still sitting in my drafts folder. :(

In addition to suggesting that you refactor update_valid_arrays out into
its own file (so that it can be used by other tests in the same folder),
I also said:

> This is fragile.  Other varargs functions (e.g., sprintf) return the
> number of parameters consumed and use a sentinel or formatting to note
> the end of the list.  Ending the list of targets with, say, 0 is much
> less susceptible to copy-and-paste bugs.  With this interface, if I
> copy-and-paste, delete an item, and forget to change num_targets, I
> lose.
>
> Something like:
>
> 	num_targets = update_valid_arrays(legal_targets,
> 					illegal_targets,
> 					ARRAY_SIZE(illegal_targets),
> 					GL_TEXTURE_1D,
> 					GL_TEXTURE_1D_ARRAY,
> 					0);

I'll check and see if there are any other comments in that message that
I haven't already repeated.

>>> +{
>>> +    va_list args;
>>> +    GLenum val;
>>> +    unsigned int i,j;
>>> +
>>> +    va_start(args, numValid);
>>> +    for (i = 0; i < numValid; i++) {
>>> +        val = va_arg(args, GLenum);
>>> +        valid[i] = val;
>>> +        /* remove the valid enum from the invalid array */
>>> +        for (j= 0; j < numInvalid; j++) {
>>> +            if (invalid[j] == val)
>>> +                    invalid[j] = 0;
>>> +        }
>>> +    }
>>> +    va_end(args);
>>> +}