<div dir="ltr">On 12 December 2013 08:29, Francisco Jerez <span dir="ltr"><<a href="mailto:currojerez@riseup.net" target="_blank">currojerez@riseup.net</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="im">Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> writes:<br>

<br>
> On 26 November 2013 00:02, Francisco Jerez <<a href="mailto:currojerez@riseup.net">currojerez@riseup.net</a>> wrote:<br>
><br>
>><br>
>> +enum glsl_image_dim {<br>
>> +   GLSL_IMAGE_DIM_1D,<br>
>> +   GLSL_IMAGE_DIM_2D,<br>
>> +   GLSL_IMAGE_DIM_3D,<br>
>> +   GLSL_IMAGE_DIM_RECT,<br>
>> +   GLSL_IMAGE_DIM_CUBE,<br>
>> +   GLSL_IMAGE_DIM_BUFFER,<br>
>> +   GLSL_IMAGE_DIM_MS<br>
>> +};<br>
>> +<br>
>><br>
><br>
> glsl_image_dim seems redundant  with glsl_sampler_dim (they are identical<br>
> except that the order is different, and glsl_sampler_dim includes the extra<br>
> value GLSL_SAMPLER_DIM_EXTERNAL).  I'd prefer to just use one enum for both<br>
> purposes, and don't create any image types using GLSL_SAMPLER_DIM_EXTERNAL.<br>
><br>
<br>
</div>This was a deliberate change.  My intention was to be able to refer to a<br>
surface dimensionality and whether it's an image or sampler type with a<br>
single identifier.  If we use the same type for both we will have to<br>
find another way for overload selection and that will probably make the<br>
notation in the builtin_type_macros.h code in PATCH 16 inconsistent with<br>
the macros for sampler types, unless we change the sampler glsl_type<br>
constructor too.<br></blockquote><div><br></div><div>It looks to me like the only function where you're taking advantage of the different enum types is to distinguish between the glsl_type constructor for samplers and images.  But the only reason that's necessary is because of a lot of other code duplication.  It seems to me that we ought to be able to coalesce:<br>
<br></div><div>glsl_type::sampler_dimensionality and glsl_type::fields.image.dimension<br></div><div>glsl_type::sampler_array and glsl_type::fields.image.array<br>glsl_type::sampler_type and glsl_type::fields.image.type<br>
<br>and then all we need to do is add a base_type argument to the existing sampler type constructor, and we can reuse it for image types.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">

<div class=""><div class="h5"><br>
> With that changed, this patch is:<br>
><br>
> Reviewed-by: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br>
</div></div></blockquote></div><br></div></div>