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

[Paul Berry]

Have you had a chance to think about any of these issues, Ian?  I'd
like to commit this patch series to Mesa.

On 5 August 2011 17:16, Paul Berry <stereotype441 at gmail.com> wrote:
> 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.
>