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

[Ian Romanick]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 08/05/2011 05:16 PM, Paul Berry 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.

It's more a matter of consistency within a file.  When all the other
comment blocks are formatted to 77-79 columns, the one block that is
formatted to 70 columns really stands out.  This is even more the case
when the formatting change happens within a single comment block.

Looking at Mesa code, I believe that everyone (tries) to wrap comments
and code to somewhere between 77 and 79 columns.  Somethings go over
(like the extension table in extensions.c), and something go under.  I
usually have my fill-column set to 78.

>>>         */
>>>        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?

"Correct" is a funny word on matters of style.  "Matches the majority of
they other switch-statements in Mesa" would be more accurate.

> 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.

I'm not a big fan of that either, and I never have been.  The mechanism
that I used before coming to open source projects was to indent the case
line.  I believe the rationale behind this formatting is so that cases
with blocks and without are at the same indentation.  There is some
merit in that.  This also means that cases that start without a block
won't have spurious whitespace changes if a block is added later.

	switch () {
	case 0:
		foo();
		break;

	case 1: {
		...
		break;
	}

	default:
		...
		break;
	}

> 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 :)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iEYEARECAAYFAk5JgH4ACgkQX1gOwKyEAw+XFwCghEsk2S9xf1MCdYHQzhFePoCU
XpEAoIyogcUBpskuAVCifHfoR1cdMNt4
=JQTj
-----END PGP SIGNATURE-----