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 <stereotype...@gmail.com> wrote:
> On 2 August 2011 18:27, Ian Romanick <i...@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.
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
