On 10 July 2010 03:47, Jason Oster jason.oster@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@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel