[Mesa-dev] [PATCH 01/10] swrast: Add LUMINANCE, INTENSITY, LUMINANCE_ALPHA to span asserts.

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 04/18/2011 08:43 PM, Brian Paul wrote:
> On Mon, Apr 18, 2011 at 8:10 PM, Eric Anholt <eric at anholt.net> wrote:
>> On Mon, 18 Apr 2011 16:16:37 -0700, Ian Romanick <idr at freedesktop.org> wrote:
>>> -----BEGIN PGP SIGNED MESSAGE-----
>>> Hash: SHA1
>>>
>>> On 04/18/2011 01:37 PM, Eric Anholt wrote:
>>>> Fixes ARB_texture_float/fbo-alphatest-formats.
>>>> ---
>>>>  src/mesa/swrast/s_readpix.c |    3 +++
>>>>  src/mesa/swrast/s_span.c    |    3 +++
>>>>  2 files changed, 6 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/src/mesa/swrast/s_readpix.c b/src/mesa/swrast/s_readpix.c
>>>> index 5604c2e..a201a63 100644
>>>> --- a/src/mesa/swrast/s_readpix.c
>>>> +++ b/src/mesa/swrast/s_readpix.c
>>>> @@ -195,6 +195,9 @@ fast_read_rgba_pixels( struct gl_context *ctx,
>>>>       rb->_BaseFormat == GL_RGB ||
>>>>       rb->_BaseFormat == GL_RG ||
>>>>       rb->_BaseFormat == GL_RED ||
>>>> +     rb->_BaseFormat == GL_LUMINANCE ||
>>>> +     rb->_BaseFormat == GL_INTENSITY ||
>>>> +     rb->_BaseFormat == GL_LUMINANCE_ALPHA ||
>>>>       rb->_BaseFormat == GL_ALPHA);
>>>
>>> At this point would it be easier to just assert the formats that are not
>>> allowed?  Is there even anything that's left as a valid _BaseFormat that
>>> isn't allowed here?
>>
>> I keep wanting to remove asserts like this, and Brian says he likes
>> them.
> 
> I'm sorry you feel inconvenienced, but I'm a big believer in assertions.

I don't think the problem is the existence of assertions.  I think the
problem is that the format assertions have gotten a bit out of control.
 They're increasingly difficult to maintain, and we often get false
negatives.  This diminishes their value significantly.

That was my point in the initial review comment.  Once the assertion is
expanded to be correct, it basically asserts that _BaseFormat must have
a value.  Since this should have been handled elsewhere, does having it
here add anything other than maintenance burden?

I also think that many of these assertions could be improved by checking
for the negative condition instead of the positive condition.  There are
far fewer values that cannot be used than there are values that can be
used.  When someone is adding support for formats GL_FOO, it is more
clear that

	assert(x != GL_FOO && x != GL_BAR);

needs to be changed than

	assert(x == GL_ASDF || x == GL_QWERTY);

> In these recent cases, perhaps we just need a better, extendable
> assertion.  Something  like
> 
> assert(_mesa_is_base_color_format(rb->_BaseFormat));

That would certainly be more maintainable for this case, but I don't
think it's a general fix.  I think someone will have to look over this
code and come up with a plan.  1, 2, 3, not it! :)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk2t07YACgkQX1gOwKyEAw8IKACcCNkd99BcCttmiBq4ekDezSED
fjkAniwnDIZPPjW1VPQNwKZyVO4xv1TK
=mzoJ
-----END PGP SIGNATURE-----