[Mesa-dev] [RFC PATCH 3/4] ff_fragment_shader: mark impossible switch values with unreachable

[Gustaw Smolarczyk]

2017-04-22 11:45 GMT+02:00 Nicolai Hähnle <nhaehnle at gmail.com>:
> On 22.04.2017 09:52, Giuseppe Bilotta wrote:
>>
>> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta at gmail.com>
>> ---
>>  src/mesa/main/ff_fragment_shader.cpp | 18 +++++++-----------
>>  1 file changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/mesa/main/ff_fragment_shader.cpp
>> b/src/mesa/main/ff_fragment_shader.cpp
>> index aac9de78ca..4f2d6b07a2 100644
>> --- a/src/mesa/main/ff_fragment_shader.cpp
>> +++ b/src/mesa/main/ff_fragment_shader.cpp
>> @@ -138,8 +138,7 @@ need_saturate( GLuint mode )
>>     case TEXENV_MODE_ADD_PRODUCTS_SIGNED_NV:
>>        return GL_TRUE;
>>     default:
>> -      assert(0);
>> -      return GL_FALSE;
>> +      unreachable("Invalid TexEnv Combine mode");
>>     }
>>  }
>>
>> @@ -426,8 +425,7 @@ get_source(texenv_fragment_program *p,
>>        }
>>
>>     default:
>> -      assert(0);
>> -      return NULL;
>> +      unreachable("Invalid TexEnv source");
>>     }
>>  }
>>
>> @@ -458,8 +456,7 @@ emit_combine_source(texenv_fragment_program *p,
>>        return src;
>>
>>     default:
>> -      assert(0);
>> -      return src;
>> +      unreachable("Invalid TexEnv Combine operand");
>>     }
>>  }
>>
>> @@ -495,8 +492,8 @@ static GLboolean args_match( const struct state_key
>> *key, GLuint unit )
>>             return GL_FALSE;
>>          }
>>          break;
>> -      default:
>> -        return GL_FALSE;       /* impossible */
>> +      default:
>> +        unreachable("Invalid TexEnv Combine operand");
>
>
> For this one, I think despite the comment it'd be best to change that to an
> assert + return first, and re-visit it after some time.

I think the comment was quite accurate - alpha argument cannot use
color values, so handling only alpha operand enums is perfectly safe.
It is being validated in texenv.c:set_combiner_operand (the second
switch); later on, the GL_* constants are converted to TEXENV_OPR ones
in texstate.c:pack_tex_combine. Though first adding an assert is safer
in case I missed something.

>
> Cheers,
> Nicolai
>
>
>>        }
>>     }
>>
>> @@ -579,9 +576,8 @@ emit_combine(texenv_fragment_program *p,
>>     case TEXENV_MODE_ADD_PRODUCTS_SIGNED_NV:
>>        return add(add(mul(src[0], src[1]), mul(src[2], src[3])),
>>                  new(p->mem_ctx) ir_constant(-0.5f));
>> -   default:
>> -      assert(0);
>> -      return src[0];
>> +   default:
>> +      unreachable("Invalid TexEnv Combine mode");
>>     }
>>  }

With Nicolai's suggestion or not:

Reviewed-by: Gustaw Smolarczyk <wielkiegie at gmail.com>

Regards,
Gustaw