<html><head></head><body><div>On Sun, 2016-01-17 at 15:20 -0800, Jason Ekstrand wrote:</div><blockquote type="cite"> On Jan 17, 2016 2:45 PM, "Timothy Arceri" <<a href="mailto:t_arceri@yahoo.com.au">t_arceri@yahoo.com.au</a>> wrote: > > On Mon, 2016-01-18 at 09:34 +1100, Timothy Arceri wrote: > > On Sun, 2016-01-17 at 08:27 -0800, Jason Ekstrand wrote: > > > > > > On Jan 16, 2016 9:15 PM, "Timothy Arceri" < > > > <a href="mailto:timothy.arceri@collabora.com">timothy.arceri@collabora.com</a>> wrote: > > > > > > > > From Section 7.9 (SUBROUTINE UNIFORM VARIABLES) of the OpenGL > > > > 4.5 Core spec: > > > > > > > > "The command > > > > > > > > void UniformSubroutinesuiv(enum shadertype, sizei count, > > > > const uint *indices); > > > > > > > > will load all active subroutine uniforms for shader stage > > > > shadertype with subroutine indices from indices, storing > > > > indices[i] into the uniform at location i. The indices for > > > > any locations between zero and the value of > > > > ACTIVE_SUBROUTINE_UNIFORM_LOCATIONS minus one which are not > > > > used will be ignored." > > > > > > > > Cc: "11.0 11.1" <a href="mailto:mesa-stable@lists.freedesktop.org">mesa-stable@lists.freedesktop.org</a> > > > > <a href="https://bugs.freedesktop.org/show_bug.cgi?id=93731">https://bugs.freedesktop.org/show_bug.cgi?id=93731</a> > > > > --- > > > > src/mesa/main/shaderapi.c | 14 ++++++++++++++ > > > > 1 file changed, 14 insertions(+) > > > > > > > > diff --git a/src/mesa/main/shaderapi.c > > > > b/src/mesa/main/shaderapi.c > > > > index cdc85f3..918c5f9 100644 > > > > --- a/src/mesa/main/shaderapi.c > > > > +++ b/src/mesa/main/shaderapi.c > > > > @@ -2530,6 +2530,13 @@ _mesa_UniformSubroutinesuiv(GLenum > > > shadertype, GLsizei count, > > > > i = 0; > > > > do { > > > > struct gl_uniform_storage *uni = sh > > > ->SubroutineUniformRemapTable[i]; > > > > + if (uni == NULL) { > > > > + if (++i < count) > > > > + continue; > > > > + else > > > > + break; > > > Just > > > i++; > > > continue; > > > Will do the same thing and without repeating the looping condition > > > or > > > making the user think about pre- vs. post-increment. I guess it > > > does > > > make them think about continues in a do-while, but you can't win > > > everything. :-) > > > > I don't feel great about imple+ if (uni == NULL) {> > > + > > if (++i < count)> > > + continue;> > > + else> > > > > + break;menting the possibility of an infinite > > loop. Sure the code above should work but this stuff doesn't really > > have extensive tests. > > > > What about at the very least: > > > > i++; > > assert(i < count); > > continue; > > Actually no that wouldn't work, the spec makes it legal to attempt to > set inactive locations there is no guarantee that any of the locations > secified by the user are active in which case your suggestion would > send them into an infinite loop.</blockquote><div> </div><blockquote type="cite"> No, it wouldn't. What I suggested is identical to what you had before. A continue in a do-while loop jumps to the branching condition, not the top of the loop. The fact that you repeat the branching condition is redundant.</blockquote><div> </div><div>My concern was that it could be possible for the last element of SubroutineUniformRemapTable to be NULL, however looking over the uniform linking code that doesn't seem possible. </div><div> </div><blockquote type="cite"> > If it makes people happy I'll just change this to: > > if (uni == NULL) { > i++; > if (i < count) > continue; > else > break; > } > > Which is what I had originally but IMO it just look ugly. > > > > > > > > > > > + } > > > > + > > > > int uni_count = uni->array_elements ? uni->array_elements > > > > : > > > 1; > > > > int j, k; > > > > > > > > @@ -2557,6 +2564,13 @@ _mesa_UniformSubroutinesuiv(GLenum > > > shadertype, GLsizei count, > > > > i = 0; > > > > do { > > > > struct gl_uniform_storage *uni = sh > > > ->SubroutineUniformRemapTable[i]; > > > > + if (uni == NULL) { > > > > + if (++i < count) > > > > + continue; > > > > + else > > > > + break; > > > > + } > > > > + > > > > int uni_count = uni->array_elements ? uni->array_elements > > > > : > > > 1; > > > > > > > > memcpy(&uni->storage[0], &indices[i], > > > > -- > > > > 2.4.3 > > > > > > > > _______________________________________________ > > > > mesa-dev mailing list > > > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > _______________________________________________ > > > mesa-dev mailing list > > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > _______________________________________________ > > mesa-dev mailing list > > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> </blockquote></body></html>