On 09/16/2013 02:20 PM, Paul Berry wrote: > On 13 September 2013 11:25, Kenneth Graunke <kenn...@whitecape.org > <mailto:kenn...@whitecape.org>> wrote: > > We now set the ARB_shader_bit_encoding flag for versions that support > this functionality, so we don't need to double check it here. > > Signed-off-by: Kenneth Graunke <kenn...@whitecape.org > <mailto:kenn...@whitecape.org>> > Cc: Ian Romanick <i...@freedesktop.org <mailto:i...@freedesktop.org>> > --- > src/glsl/builtin_functions.cpp | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/src/glsl/builtin_functions.cpp > b/src/glsl/builtin_functions.cpp > index c468bd5..b020a7c 100644 > --- a/src/glsl/builtin_functions.cpp > +++ b/src/glsl/builtin_functions.cpp > @@ -182,8 +182,7 @@ shader_texture_lod_and_rect(const > _mesa_glsl_parse_state *state) > static bool > shader_bit_encoding(const _mesa_glsl_parse_state *state) > { > - return state->is_version(330, 300) || > - state->ARB_shader_bit_encoding_enable || > + return state->ARB_shader_bit_encoding_enable || > state->ARB_gpu_shader5_enable; > } > > -- > 1.8.3.4 > > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org <mailto:mesa-dev@lists.freedesktop.org> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > > > I'm not a huge fan of this approach. We're currently inconsistent in > Mesa as to how we handle GLSL extensions that were adopted into later > versions of GLSL (or backports from later versions of GLSL). For these > extensions: > > ARB_draw_instanced > ARB_fragment_coord_conventions > ARB_gpu_shader5 > ARB_shader_texture_lod > ARB_shading_language_420pack > ARB_shading_language_packing > ARB_texture_cube_map_array > ARB_texture_multisample > OES_texture_3D > > we use what I'll call the "enable means explicitly enabled" style, which > means we only set the "_enable" flag if the shader contains text like > "#extension ARB_foo: enable"; if the extension's functionality is > provided by the given version of GLSL, we fold that into the if-test > that enables the functionality--e.g.: > > if (state->ARB_draw_instanced_enable || state->is_version(140, 300)) > add_system_value(SYSTEM_VALUE_INSTANCE_ID, int_t, "gl_InstanceID"); > > But for these extensions: > > ARB_explicit_attrib_location > ARB_texture_rectangle > > we use what I'll call the "enable means available" style, which means > that we set the "_enable" flag when processing the version directive (or > in the _mesa_glsl_parse_state constructor), to indicate that the > functionality is available, whether or not the user explicitly requested > it or not. > > (Note: for ARB_uniform_buffer_object it looks like we do some of each > style!) > > > Personally I'd prefer to see us consistently adopt the "enable means > explicitly enabled" style (to me, the "enable means available" style > feels like we're fibbing to ourselves). An additional advantage of the > "enable means explicitly enabled" style is that it allows us to handle > cases (such as in ARB_draw_instanced) where the extension differs > slightly from the functionality that was eventually folded into GLSL. > Another advantage is that if a client-supplied shader does something > silly like "#extension ARB_draw_instanced: disable" in a GLSL 1.40+ > shader, we won't accidentally disable built-in functionality (although > in practice this is extremely unlikely to ever crop up). > > If it becomes too onerous to add the "|| state->is_version(...)" checks > all over the place we can always make some inline functions, e.g.: > > struct _mesa_glsl_parse_state { > ... > bool is_draw_instanced_available() const > { > return this->ARB_draw_instanced_enable || this->is_version(140, 300); > } > ... > };
I also like this style a lot better than mashing the extension enables. Now that you, Ian, and I are all in agreement, I'll plan on writing some new patches that switch to this style. --Ken _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
