Since the Scintilla C++ lexer started to fold on `()` [1], the code looking up the current scope is confused whenever the function signature spans multiple lines. Fix this by resolving matching parentheses on the candidate line before checking the fold level that follows actually contains the current line.
Fixes #1279.
[1] https://sourceforge.net/p/scintilla/feature-requests/1138/ imported in 24f91981c057a7e212c09da66fb974c3ccc85bd6 You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1280
-- Commit Summary --
* Fix the current scope shown in the statusbar
-- File Changes --
M src/symbols.c (36)
-- Patch Links --
https://github.com/geany/geany/pull/1280.patch https://github.com/geany/geany/pull/1280.diff
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
Since these are all the same type, they could be declared in the scope in which they're used (the for loop).
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci); + + for (; pos < line_end; pos++) {
Missing newline before brace
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace)
The `brace` parameter could be a `gchar` even though it's passed an int (char constant) below, it's always ASCII `(` and compared below against a `char` returned from `sci_get_char_at()`. No big deal though, obviously, just a `gchar` is more readable since it's semantically a character and not an int (which is used in Geany for lots of other stuff, sizes, byte offsets, enums, etc).
elextr commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
Multiple declarations in the `for`? is that in the styles allowed? :smile:
But certainly `pos` should be confined to the loop.
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
Following usually [best practices](https://www.securecoding.cert.org/confluence/display/c/DCL19-C.+Minimize+the...) they all should be really. It's not a common style in Geany yet because we only recently started using C99, but IMO we should follow this style wherever possible.
elextr commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci); + + for (; pos < line_end; pos++) {
This assumes the open parens is on the tag line I presume? If so its style dependent, because C doesn't require it to be on the same line. Maybe should look for the first open parens not assume its on the same line.
b4n commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
Do we really want something like that? ```C
for (gint pos = sci_get_position_from_line(sci, line), line_end = sci_get_line_end_position(sci, line), lexer = sci_get_lexer(sci); pos < line_end; pos++) ```
I mean, sure in theory scoping is best, but here IMO it makes this harder to read, and the parent scope doesn't contain anything anyway, so it's not mixed with other stuff. If it was a `size_t i`, sure, but here I'm not convinced.
b4n commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci); + + for (; pos < line_end; pos++) {
@codebrainz indeed, will fix
b4n commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace)
will fix
b4n commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci); + + for (; pos < line_end; pos++) {
@elextr
does this need to be a separate function?
No, I initially wrote it inside `get_current_tag_name()`'s body, but it seemed like a contained enough logic to be worth wrapping in a logical container (the function). I can un-split it if you want, but it seemed slightly less nice to me there. But admittedly I'm an advocate of splitting up logical units when the reader can then see it as that logical unit ("this search for the line on which unmatched parentheses on `@line` are all matched") and can avoid caring about the implementation, or find a implementation issue in the logic there rather than in the context of a whole other set of stuff -- but yes, I do think there is such thing as over-splitting, too (basically when it ends up in split things that don't have a defined goal by themselves).
its style dependent, because C doesn't require it to be on the same line. Maybe should look for the first open parens not assume its on the same line.
That's a good point, but would be slightly more subtle to fix. Though maybe not so much, just check each fold level between the tag and the containing one and see if it matches parentheses matching, and skip it if so. I'll give it a look.
elextr commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
@b4n, IMHO declare pos in the loop, its the loop iterator, thats a common paradigm.
@b4n pushed 1 commit.
3c566e6 fixups
elextr commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci); + + for (; pos < line_end; pos++) {
but yes, I do think there is such thing as over-splitting, too (basically when it ends up in split things that don't have a defined goal by themselves).
Its always a judgement call, and it depends critically on the name of the function being obvious, otherwise everybody has to go "oh, `resolve_brace_match` hmmm, what does that do? what does "resolve" mean in this context? I guess I better go find its definition and read it. Which is a waste of everybodys time instead of having a few extra lines in the function, which you can even comment :)
@b4n pushed 2 commits.
d6e69c9 declare pos inside the for loop 939d055 Rewrite fix for finding the current scope when there is folds on parentheses
Rewritten differently to address the issue where it only solved the case where the argument list starts on the tag line (which is highly likely, but not guaranteed) as per @elextr comment.
Now, it skips fold levels that look like being introduced by parentheses. It might be slightly problematic if a language doesn't fold on parentheses yet has a fold level that so happens to match perfectly just as if it did. I doubt we would see that, but if it's a problem we could restrict the extra checks to `LexCPP` (or any other that folds on `()`).
LGBI
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
Do we really want something like that?
Looks OK to me, what's hard to read about it?
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
IMHO declare pos in the loop, its the loop iterator, thats a common paradigm
Declaring the loop terminator in it is also common.
b4n commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
Looks OK to me, what's hard to read about it?
I find this overlong precondition hides the logic of the loop (condition and post-body).
Anyway, the new code is different, do you still think I should so something like that now the 2 other variables are constant?
elextr commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
@b4n, agree that them being const is correct, and assists the compiler (and human) in understanding your intention. Its not possible to do that in the initialiser if `pos` is non-const, so better out of it.
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
I find this overlong precondition hides the logic of the loop (condition and post-body).
You could insert another line break instead of tucking it at the end of the last initializer line.
Anyway, the new code is different, do you still think I should so something like that now the 2 other variables are constant?
My opinion is that doing so should be the default unless there is a reason not to, for example to keep it out of the loop for performance reasons, or I suppose in this case to have them be `const`. That being said, using `const` like that is also not used often in Geany and is something else that should either be done all the time or not. I think we should update `HACKING` to indicate both of these (though I'm not convinced we want to sprinkle `const` everywhere, personally).
elextr commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
`const` is good, `const` should be used more :-D
That said, nobody should sit down and add it to existing code, just use it on new code when you can.
Modifying existing code can be problematical, passing a `const` to existing functions with non-const parameters would cascade `const` changes everywhere through code that didn't need to be modified. So its not worth it to change old code unless its local only.
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
I propose something like the following changes to `HACKING`:
```diff diff --git a/HACKING b/HACKING index 938688d..13e4025 100644 --- a/HACKING +++ b/HACKING @@ -200,8 +200,18 @@ Coding moment, we want to keep the minimum requirement for GTK at 2.24 (of course, you can use the GTK_CHECK_VERSION macro to protect code using later versions). -* Variables should be declared before statements. You can use - gcc's -Wdeclaration-after-statement to warn about this. +* Variables should be declared (and initialized) as close as practical + to their first use. This reduces the chances of intervening code being + inserted between declaration and use, where the variable may be + uninitialized. +* Variables should be defined within the smallest scope that is practical, + for example inside a conditional branch which uses them or in the + initialization part of a for loop. +* Local variables that will not be modified should be marked as ``const`` + to indicate intention. This allows the compiler to give a warning if + part of the code accidentally tries to change the value. This does not + apply to by-value parameters where it needlessly exposes the + implementation and it's obvious a copy is made anyway. * Don't let variable names shadow outer variables - use gcc's -Wshadow option. * Do not use G_LIKELY or G_UNLIKELY (except in critical loops). These ```
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
s/by-value/non-pointer/
elextr commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
Looks basically ok. Can you make a PR for the HACKING changes instead of it being a comment on an outdated commit on an unrelated PR :)
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
I can, I just wanted to see what you guys thought before making a PR that will have to be changed a bunch of times, and put it here with the current discussion (which is available in the comments mailing list for posterity and emailed to anyone interested as well) rather than spreading it all over.
codebrainz commented on this pull request.
@@ -2289,6 +2290,32 @@ static gint get_fold_header_after(ScintillaObject *sci, gint line)
}
+/* returns the line after following all brace match for @brace on @line */ +static gint resolve_matching_braces(ScintillaObject *sci, gint line, gint brace) +{ + gint pos = sci_get_position_from_line(sci, line); + gint line_end = sci_get_line_end_position(sci, line); + gint lexer = sci_get_lexer(sci);
See #1282
Quick test seems to work.
Didn't I merge that already? damned, I didn't.
Merged #1280.
Thanks 🥇
github-comments@lists.geany.org