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 <j...@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); >>> +} _______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit