[Mesa-dev] [PATCH 7/9] glcpp: pass gl_context to preprocess()

[nobled]

On Fri, Apr 13, 2012 at 2:04 PM, Ian Romanick <idr at freedesktop.org> wrote:
> On 04/13/2012 08:52 AM, nobled wrote:
>>
>> ---
>>  src/glsl/glcpp/glcpp.c        |    2 +-
>>  src/glsl/glcpp/glcpp.h        |    4 +++-
>>  src/glsl/glcpp/pp.c           |    3 ++-
>>  src/glsl/glsl_parser_extras.h |    2 +-
>>  src/glsl/linker.cpp           |    2 --
>>  src/glsl/main.cpp             |    2 +-
>>  src/glsl/test_optpass.cpp     |    2 +-
>>  7 files changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/src/glsl/glcpp/glcpp.c b/src/glsl/glcpp/glcpp.c
>> index e461a65..bd6344b 100644
>> --- a/src/glsl/glcpp/glcpp.c
>> +++ b/src/glsl/glcpp/glcpp.c
>> @@ -111,7 +111,7 @@ main (int argc, char *argv[])
>>        if (shader == NULL)
>>           return 1;
>>
>> -       ret = preprocess(ctx,&shader,&info_log, NULL, API_OPENGL);
>> +       ret = preprocess(ctx,&shader,&info_log, API_OPENGL, NULL);
>>
>>
>>        printf("%s", shader);
>>        fprintf(stderr, "%s", info_log);
>> diff --git a/src/glsl/glcpp/glcpp.h b/src/glsl/glcpp/glcpp.h
>> index 2d7cad2..0c52077 100644
>> --- a/src/glsl/glcpp/glcpp.h
>> +++ b/src/glsl/glcpp/glcpp.h
>> @@ -160,6 +160,8 @@ typedef struct active_list {
>>        struct active_list *next;
>>  } active_list_t;
>>
>> +struct gl_context;
>> +
>>  struct glcpp_parser {
>>        yyscan_t scanner;
>>        struct hash_table *defines;
>> @@ -192,7 +194,7 @@ glcpp_parser_destroy (glcpp_parser_t *parser);
>>
>>  int
>>  preprocess(void *ralloc_ctx, const char **shader, char **info_log,
>> -          const struct gl_extensions *extensions, int api);
>> +          int api, struct gl_context *glctx);
>>
>>  /* Functions for writing to the info log */
>>
>> diff --git a/src/glsl/glcpp/pp.c b/src/glsl/glcpp/pp.c
>> index b0afed4..7218eeb 100644
>> --- a/src/glsl/glcpp/pp.c
>> +++ b/src/glsl/glcpp/pp.c
>> @@ -146,9 +146,10 @@ remove_line_continuations(glcpp_parser_t *ctx,
>> const char *shader)
>>
>>  int
>>  preprocess(void *ralloc_ctx, const char **shader, char **info_log,
>> -          const struct gl_extensions *extensions, int api)
>> +          int api, struct gl_context *glctx)
>>  {
>>        int errors;
>> +       struct gl_extensions *extensions = glctx ?&glctx->Extensions :
>> NULL;
>
>
> It looks like the only place that can call preprocess with a NULL extension
> pointer is the standalone preprocessor.  I think it would be better to
> modify that code to pass an empty gl_context with the gl_extensions field
> cleared to zeros.  I think that would simplify this code a bit, and it would
> eliminate the need to have a separate api parameter (preprocess could just
> look at the gl_context).
Yeah, I wanted to do that at first, but...
>
> Some helpers like in standalone_scaffolding.cpp might be in order.  This
> would match the way the standalone GLSL compiler works.
Yeah, it looked like it required duplicating
initialize_context_to_defaults() and initialize_context() from
main.cpp and standalone_scaffolding.cpp. Can we just move these down a
layer into a new file, glcpp_standalone.cpp? The standalone
preprocessor is already duplicating the _mesa_reference_shader(), and
now the _mesa_shader_error(), standalone functions.

>
> As a side comment, we should probably get the glcpp unit tests wired into
> 'make check'.
>
>
>>        glcpp_parser_t *parser = glcpp_parser_create (extensions, api);
>>        *shader = remove_line_continuations(parser, *shader);
>>
>> diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
>> index 1a909c6..10402c9 100644
>> --- a/src/glsl/glsl_parser_extras.h
>> +++ b/src/glsl/glsl_parser_extras.h
>> @@ -297,7 +297,7 @@ extern "C" {
>>  #endif
>>
>>  extern int preprocess(void *ctx, const char **shader, char **info_log,
>> -                      const struct gl_extensions *extensions, int api);
>> +                      int api, struct gl_context *glctx);
>>
>>  extern void _mesa_destroy_shader_compiler(void);
>>  extern void _mesa_destroy_shader_compiler_caches(void);
>> diff --git a/src/glsl/linker.cpp b/src/glsl/linker.cpp
>> index e2a5ef9..e43ec3a 100644
>> --- a/src/glsl/linker.cpp
>> +++ b/src/glsl/linker.cpp
>> @@ -175,10 +175,8 @@ private:
>>  static void
>>  linker_msg(gl_shader_program *prog, bool error, const char *fmt, va_list
>> ap)
>>  {
>> -
>>     ralloc_strcat(&prog->InfoLog, error ? "error: " : "warning: ");
>>     ralloc_vasprintf_append(&prog->InfoLog, fmt, ap);
>> -
>
>
> Eh... maybe just don't introduce these whitespace errors in the earlier
> patch (6/9)? :)
Whoops, heh. Should not use `git add -u`.
>
>>  }
>>
>>
>> diff --git a/src/glsl/main.cpp b/src/glsl/main.cpp
>> index d43bf1a..1d509eb 100644
>> --- a/src/glsl/main.cpp
>> +++ b/src/glsl/main.cpp
>> @@ -136,7 +136,7 @@ compile_shader(struct gl_context *ctx, struct
>> gl_shader *shader)
>>
>>     const char *source = shader->Source;
>>     state->error = preprocess(state,&source,&state->info_log,
>>
>> -                            state->extensions, ctx->API) != 0;
>> +                            ctx->API, ctx) != 0;
>>
>>     if (!state->error) {
>>        _mesa_glsl_lexer_ctor(state, source);
>> diff --git a/src/glsl/test_optpass.cpp b/src/glsl/test_optpass.cpp
>> index 6abafb5..0844706 100644
>> --- a/src/glsl/test_optpass.cpp
>> +++ b/src/glsl/test_optpass.cpp
>> @@ -219,7 +219,7 @@ int test_optpass(int argc, char **argv)
>>        shader->Source = input.c_str();
>>        const char *source = shader->Source;
>>        state->error = preprocess(state,&source,&state->info_log,
>>
>> -                                state->extensions, ctx->API) != 0;
>> +                                ctx->API, ctx) != 0;
>>
>>        if (!state->error) {
>>           _mesa_glsl_lexer_ctor(state, source);
>
>