[Mesa-dev] [PATCH 3/4] glsl: Create a standalone executable for testing optimization passes.

[Paul Berry]

On 19 July 2011 13:15, Ian Romanick <idr at freedesktop.org> wrote:
> We'll probably have to tweak the build a bit to be sure everything ends
> up in the tarballs.  Since José's changes recently this may not be as
> much of a problem, but we'll at least want to test it.  We can do that
> on Wednesday.

Ok, cool.  Are the tarballs meant to contain both source and
installation artifacts, or just installation artifacts?  Because the
unit tests are only intended to be run via "make check"--they aren't
intended to be part of the final install.

>> +static const char *extract_command_from_argv(int *argc, char **argv)
>> +{
>> +   if (*argc < 2) {
>> +      usage_fail(argv[0]);
>> +   }
>> +   const char *command = argv[1];
>> +   --*argc;
>> +   memmove(&argv[1], &argv[2], (*argc) * sizeof(argv[1]));
>> +   return command;
>> +}
>
> This feels like an odd re-invention of getopt.  Why?

My intention was to allow the glsl_test executable to exhibit
significantly different functionality (including taking different
options) based on its first argument, much in the same way that "git
log" takes different options from "git commit".  I'm not aware of a
way to support this functionality directly in getopt, so what I did is
used extract_command_from_argv() to extract the first argument, and
then based on that argument I decide which behavior to invoke, and
that behavior uses getopt to parse whatever particular options it
supports.

There's a bit of planning-for-the-future going on here, since at the
moment glsl_test only supports one possible value for its first
argument, namely "optpass".

>> +static struct gl_shader *
>> +_mesa_new_shader(struct gl_context *ctx, GLuint name, GLenum type)
>> +{
>> +   struct gl_shader *shader;
>> +
>> +   (void) ctx;
>> +
>> +   assert(type == GL_FRAGMENT_SHADER || type == GL_VERTEX_SHADER);
>> +   shader = rzalloc(NULL, struct gl_shader);
>> +   if (shader) {
>> +      shader->Type = type;
>> +      shader->Name = name;
>> +      shader->RefCount = 1;
>> +   }
>> +   return shader;
>> +}
>
> Another candidate for refactoring. :)  Since this can't go in
> glsl_parser_utils.cpp, the common bits of scaffolding should probably go
> in their own file.  Perhaps src/glsl/standalone_scaffolding.cpp?

Yeah, true.  I'll take care of this.

>
> It looks like there's a bit of code below for reading the shader file
> that could use the same treatment.

I assume you're talking about read_stdin_to_eof(), below.  By "the
same treatment" do you mean "moving into
src/glsl/standalone_scaffolding.cpp" or "avoiding code duplication
with the builtin compiler"?  I can certainly move it into
scandalone_scaffolding.cpp, but it doesn't duplicate things the
builtin compiler does because the builtin compiler reads from a file,
and it determines the file's length by seeking to the end--that won't
work on stdin.  (The reason I had test_optpass.cpp read from stdin was
so that the unit tests wouldn't have to create temporary files).

>
>> +
>> +static string read_stdin_to_eof()
>> +{
>> +   stringbuf sb;
>> +   cin.get(sb, '\0');
>> +   return sb.str();
>> +}
>
> Why are you mixing C++ iostreams and C FILE I/O?  That seems weird.  I'd
> prefer to stick with C FILE I/O because everybody on the project knows
> that well, but the real preference is to not mix and match.

I realize I'm at risk of starting a feud here, but in general I prefer
C++ library functions because they are less vulnerable to bugs like
buffer overruns.  I've been trying to restrain myself from using too
many C++ idioms, since they seem to be frowned upon in Mesa, but in
this particular case, the C++ version is so much more straightforward
than the C version that it seemed worth rocking the boat.  The C
version of this function, if I'm not mistaken, looks something like
this:

static char *read_stdin_to_eof()
{
  char buffer[4096] = "\0";
  size_t len = 0;
  char *result = (char *) malloc(1);
  while(fgets(buffer, sizeof(buffer), stdin))
  {
    size_t bytes_read = strlen(buffer);
    result = (char *) realloc(result, len + bytes_read + 1);
    memcpy(result + len, buffer, bytes_read);
    len += bytes_read;
  }
  result[len] = '\0';
  return result;
}

There's a number of places where you need to be careful: making sure
to allocate result before the while loop (so that if the first call to
fgets() returns NULL we won't segfault), remembering to leave room for
the null terminator when realloc-ing the result, remembering to tack
the null terminator on at the end of the function, and remembering to
have the caller free the memory.  The C++ version is less than half
the length and has none of these subtleties.

Since your primary concern is mixing and matching between the use of C
and C++ libraries, I would argue for moving toward C++ libraries,
since they are less prone to bugs.

As to people knowing C libraries better than C++ libraries, that is
going to become less and less true over time as C++ ages and matures,
and people start joining the project who learned the C++ library
functions before the C ones.  In fact, I would argue that the turnover
point has already occurred: the C++ iostream functions have been
around for 25 years, and they were being taught in preference to the C
functions when I was a freshman in college in 1994.