[Mesa-dev] [PATCH 1/5] glsl: Move get_error_instruction() from ir_call to ir_constant.

[Kenneth Graunke]

On 09/21/2011 10:21 AM, Eric Anholt wrote:
> On Tue, 20 Sep 2011 18:28:15 -0700, Kenneth Graunke <kenneth at whitecape.org> wrote:
>> All this does is generate a bogus value with error type; the fact that
>> it was in ir_call was rather arbitrary to begin with. ir_constant is an
>> equally arbitrary place. The rationale is that a future commit will
>> change ir_calls from rvalues to statements, and all uses of this
>> function expect an rvalue.
> 
> This would make a lot more sense to me as a global
> "get_error_rvalue(ctx)" instead of a method of some arbitrary class.

I agree that putting it in ir_constant is quite arbitrary, but I haven't
found a better solution.  It has to be an instance of _some_ class; I
chose ir_constant since it's a leaf node (well, except for
structures/arrays, I guess.)

Here are some options I've thought about:

1. Make it an instance of ir_rvalue itself.

   The problem here is that ir_rvalue can't be instantiated, as it has
pure virtual methods (clone, constant_expression_value).  I could
implement those (clone would make a new error_type object, CE would
return NULL).  But this feels dirty; I'd really rather keep it as an
abstract base class.

2. Create a new ir_error subclass of ir_rvalue.

   This is clean, but aggrandizes something that's used so little.
   I'm OK with doing this if you'd prefer it.

I'm honestly not a fan of error_type.  We've had a lot of crashes due to
someone inserting half-baked IR (i.e. ir_expression with NULL operands)
with type set to error_type.  It'd probably be nicer to have a leaf-node
"error value" singleton.  Maybe an ir_error class would be a step in the
right direction (though we'd also need to make ir_rvalues not exec_nodes
too).

Still, I think ir_constant is at least better than ir_call.  Should we
go with that for now, or, what would you prefer?