[Mesa-dev] [PATCH 3/3] glsl: Perform implicit type conversions on function call out parameters.

[Paul Berry]

On 2 August 2011 18:27, Ian Romanick <idr at freedesktop.org> wrote:
>> +       *
>> +       * Also, perform implicit conversion of arguments.  Note: to
>> +       * implicitly convert out parameters, we need to place them in a
>> +       * temporary variable, and do the conversion after the call
>> +       * takes place.  Since we haven't emitted the call yet, we'll
>> +       * place the post-call conversions in a temporary exec_list, and
>> +       * emit them later.
>
> It looks like this comment (and the others below) are wrapped to much
> less than 80 columns.

They are wrapped to 70 columns, which is the default value used by
Emacs and appeals to me personally.  But I care way less about
formatting details like this than I care about not losing time to
debating them, and I think the best way to avoid that is to pick a
standard and document it.  What's your preferred line-wrapping width,
and would you be willing to enshrine it in docs/devinfo.html (where it
might, I fear, be subject to a round of bike-shedding by those with
strong feelings about such things)?  If so, I'd be glad to update my
Emacs configuration to follow it.

>
>>         */
>>        exec_list_iterator actual_iter = actual_parameters->iterator();
>>        exec_list_iterator formal_iter = sig->parameters.iterator();
>> @@ -185,8 +194,50 @@ match_function_by_name(exec_list *instructions, const char *name,
>>        }
>>
>>        if (formal->type->is_numeric() || formal->type->is_boolean()) {
>> -         ir_rvalue *converted = convert_component(actual, formal->type);
>> -         actual->replace_with(converted);
>> +            switch (formal->mode) {
>> +            case ir_var_in:
>> +               {
>
> The { should be on the same line as the case, and emacs got the
> indentation wonkey because it's not.

For the record, this was actually completely on purpose.  I prefer for
an opening brace after a case label to be on its own line, and to
cause an extra level of indent, to emphasize the fact that the brace
exists solely for the purpose of variable scoping, and is not part of
the syntax of the switch statement (this is in contrast with the {
that comes after an if statement, for example, which is part of the
syntax of "if" and not solely for scoping).  Incidentally, the
behavior of "indent -br -i3 -npcs --no-tabs" (which is recommended by
devinfo.html) seems to agree with my formatting--it doesn't alter this
case block as I submitted it, though it does force the opening braces
of if-statements to be on the same line as the "if".  And if I force
the { to the same line as the case, the "indent" command continues to
generate the indentation that appears "wonky" to you.

I'm guessing from looking at code elsewhere in Mesa that what you
would have preferred would have been this, correct?

switch (formal->mode) {
case ir_var_in: {
   ir_rvalue *converted
      = convert_component(actual, formal->type);
   actual->replace_with(converted);
   break;
}
case ir_var_out:
   ...
}

Normally I wouldn't bike-shed something like this, but what bothers me
about the above is that it puts the closing brace of the case at the
same indentation as the closing brace of the switch statement.  That
makes it hard to tell at a glance which closing brace terminates the
switch statement and which closing brace terminates the case, and that
is IMHO the whole point of indentation.

However, having said all that, I realize that I'm still fairly new to
the mesa project and I may just have to suck it up and conform to the
mesa rules.  I just wanted to give you a chance to change your mind :)

>
>> +                  ir_rvalue *converted
>> +                     = convert_component(actual, formal->type);
>> +                  actual->replace_with(converted);
>> +               }
>> +               break;
>> +            case ir_var_out:
>> +               if (actual->type != formal->type)
>> +               {
>
> The { should be on the same line as the if.

_This_ was a complete oversight on my part.  I'll fix it.

>
>> +                  /* Create a temporary variable to hold the out
>> +                   * parameter.  Don't pass a hashtable to clone()
>> +                   * because we don't want this temporary variable to
>> +                   * be in scope.
>> +                   */
>> +                  ir_variable *tmp = formal->clone(ctx, NULL);
>> +                  tmp->mode = ir_var_auto;
>
> ir_var_auto is only used for user-defined variables, and
> ir_var_temporary is used for compiler-generated variables.  We'd also
> usually do something like:
>
>    ir_variable *tmp =
>        new(mem_ctx) ir_variable(formal->type,
>                                 "out_parameter_conversion",
>                                 ir_var_temporary);
>
> Giving it a name like that makes the origin of the variable clear in
> shader dumps.  This has proven to be an invaluable debugging tool.

That seems very reasonable.  I'll make that change.