On 4 November 2011 16:43, Marek Olšák <<a href="mailto:maraeo@gmail.com">maraeo@gmail.com</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;"> Hi Paul, I won't comment on the patch 1, because I didn't write it, I was just preserving history. On Fri, Nov 4, 2011 at 10:00 PM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: [snip] <div><div class="h5">>> +bool parse_tfeedback_decl(const void *mem_ctx, const char *input, >> + struct tfeedback_decl *decl) >> +{ >> + /* We don't have to be pedantic about what is a valid GLSL variable >> name, >> + * because any variable with an invalid name can't exist in the IR >> anyway. >> + */ >> + >> + const char *bracket = strrchr(input, '['); >> + >> + if (bracket) { >> + decl->name = ralloc_strndup(mem_ctx, input, bracket - input); >> + if (sscanf(bracket, "[%u]", &decl->array_index) == 1) { >> + decl->is_array = true; >> + return true; /* Found: "var[i]" */ >> + } >> + } else { >> + decl->name = ralloc_strdup(mem_ctx, input); >> + return true; >> + } >> + >> + return false; >> +} > > For non-arrays, this function doesn't assign to decl->is_array or > decl->array_index. That's a problem, because its caller > (ir_set_transform_feedback_outs::ir_set_transform_feedback_outs()) passes > uninitialized memory for decl. We could fix the problem by having > ir_set_transform_feedback_outs::ir_set_transform_feedback_outs() initialize > the memory first, but I would prefer if we fixed the problem by always > assigning to all fields of decl, since we're effectively treating it as an > output of this function. </div></div>Yeah, that was unintentional. Regarding the other comments, I don't know the GLSL compiler so well. I had forked ir_set_program_inouts and was basing upon that. Now that I think about it, it makes more sense to do it like you say. :) Concerning this: varying mat4 r; glTransformFeedbackVaryingsEXT(.. "r[1]" ..); The EXT spec doesn't state whether this is legal. It says nothing about matrices, actually. [snip] <div class="im">> > The patch series doesn't add any code that calls this function. Is that > planned for a future patch? > </div>The function is already used here: <a href="http://cgit.freedesktop.org/%7Emareko/mesa/log/?h=transform-feedback" target="_blank">http://cgit.freedesktop.org/~mareko/mesa/log/?h=transform-feedback</a> It's a work in progress, but EXT_transform_feedback already works quite well and ARB_transform_feedback2 too except glDrawTransformFeedback, which isn't fully implemented yet. Despite your remarks, the GLSL linker works pretty good for me, so I am going to take a break from it for a while. Feel free to take over all 3 patches and implement it properly, as I might not have time to get to it again soon. </blockquote><div> Ok, I've worked up an alternative proposal, which I'll send out shortly. </div></div>