[Geany-devel] What's with the for loop in get_brace_indent() ?

[Lex Trotman]

On 10 July 2010 03:47, Jason Oster <jason.oster at campnavajo.com> wrote:
> Greetings list!
>
> While doing some preliminary work on a simple patch, I noticed some
> interesting code in editor.c:get_brace_indent() svn r5100.
>
> It starts out simple enough, with a for loop iterating over the full length
> of the current line of text at the caret position.  The first thing tested
> though does not make sense to me:
>
>> if (linebuf[i] == '{' && i == (len - 1))
>
> It checks the character currently being iterated through, and *then*
> requires that it's the last character in the line.  :(
>
> Assuming this fails (the last character in the line is not '{'), the code
> does something else that is strange:
>
>> gint k = len - 1;
>> while (k > 0 && isspace(linebuf[k])) k--;
>
> If finds the last non-white space character in the line ...
>
>> if (linebuf[k] == '{')
>
> ... and verifies that it's an opening curly brace.  And this all happens
> within a for loop that iterates forward through linebuf[].
>
> "But that's not all!"
>
> The for loop is unconditionally broken on the first iteration!
>
> As a result, the for loop is useless, the ret++ is useless (the function
> only returns 0 or 1), and the first check (against the last character) is
> useless.  As a result, I recommend the following patch to clean up this
> function.

This keeps it working as is but I believe that the current operation is wrong.

The requirement that the brace be the last non-whitespace character is
there for a specific Perl idiom (according to the comment) but it
causes a problem that indenting doesn't work if there is anything on a
line after the {, for example a comment, or code with a different
style that allows statements after braces.  So in all these cases I
have to adjust the indenting manually.  This has been annoying me for
some time but I havn't had time to look for it.

Brace indenting shouldn't be depending on coding style (there is lots
of old code and I'm not going to re-style it all).

It should check for an unmatched non-comment (check the sytle) brace.
That will fix the perl problem as well.

>
> I left the return value gint, rather than making it a gboolean.  The only
> code I know of that uses this function
> (editor.c:get_indent_size_after_line()) multiplies the return value against
> the indent width.  On the other hand, what the heck?  I might just opt to
> replace that usage with something like this:
>
>> size += (get_brace_indent(sci, line) ? iprefs->width : 0);

I'd recommend making it gboolean if you do this for clarity


>
> That's actually kind of pretty.  ;)  Does anyone know if get_brace_indent()
> has other uses? It doesn't look like it's exported to plugins, for example,
> and grep doesn't find anything else.

I'd consider grep definitive, so long as you searched for the right thing. :-)

Cheers
Lex

>
> Thanks list!
>
> _______________________________________________
> Geany-devel mailing list
> Geany-devel at uvena.de
> http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
>
>