* HACKING: Avoid untyped pointers & `*_foreach()` with non-NULL `user_data` void pointer where practical. * build.c: Avoid `g_ptr_array_foreach` with `user_data` void pointer - this also means we can get rid of the singleton `ForEachData`, which simplifies the code quite a bit.
Note: `GPtrArray` is already not typesafe, but it does support external iteration, which allows us to avoid passing the address of typed data to a `user_data` void pointer, which is another violation of type safety. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2270
-- Commit Summary --
* build.c: Remove g_ptr_array_foreach with untyped user_data * HACKING: Avoid untyped pointers & *_foreach() with non-NULL user_data
-- File Changes --
M HACKING (4) M src/build.c (41)
-- Patch Links --
https://github.com/geany/geany/pull/2270.patch https://github.com/geany/geany/pull/2270.diff
ntrel commented on this pull request.
{
- GKeyFile *config; - GPtrArray *ft_names; -} ForEachData; - - -static void foreach_project_filetype(gpointer data, gpointer user_data) -{ - GeanyFiletype *ft = data; - ForEachData *d = user_data;
C really should require a cast for these to highlight the unsafety.
codebrainz commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
IMO this would read better as:
```c GPtrArray *build_fts = pj->priv->build_filetypes_list; for (guint i = 0; i < build_fts->len; i++) { ft = build_fts->pdata[i]; /// other code here... } ```
so it doesn't hide code behind a macro. It's basically the same amount of code, and index iteration is something everyone already knows without thinking about it, unlike the macro that is named like a function.
elextr commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
Agree with @codebrainz lower case local macros is BAD, please don't perpetuate the evil Geany already has.
@ntrel pushed 1 commit.
c23ec96c1b1744882dd2208d9e5cf90fdd2b8a64 Avoid using foreach_ macro
ntrel commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
@codebrainz @elextr I patched in the pointer indexing code. We should add `@deprecated` to all the `foreach_*` macro doc-comments in the API, if they're not supposed to be used in new code, so people have a chance to find out.
ntrel commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
lower case local macros is BAD
There are 6 of these in build.c, I can't work out who named them (it might need bisecting so obviously not worth it).
codebrainz commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
We should add @deprecated to all the foreach_* macro doc-comments in the API, if they're not supposed to be used in new code, so people have a chance to find out.
:+1:
I don't think there's been any discussion about it (lately) but my personal opinion is that all macros and functions which obscure what the code does just to save a line or two of code should be deprecated. C is verbose, but it's easy to read if the code is all in sequence.
codebrainz commented on this pull request.
@@ -219,6 +219,10 @@ Coding
``gint``, use a ``gchar`` for individual (ASCII/UTF-8) string characters rather than ``gint``, and use a ``guint`` for integers which cannot be negative rather than ``gint``. +* Avoid using untyped pointers (e.g. gpointer) where practical. +* Prefer loops to calling *_foreach() with a non-NULL untyped + ``user_data`` pointer parameter, unless external iteration is not + supported (e.g. for tree structures).
This is a bit confusing IMO. Maybe it would read better by just dropping the `with a non-NULL untyped user_data pointer parameter`? Even without that part, it's still a useful guideline and not overly specific.
codebrainz commented on this pull request.
@@ -219,6 +219,10 @@ Coding
``gint``, use a ``gchar`` for individual (ASCII/UTF-8) string characters rather than ``gint``, and use a ``guint`` for integers which cannot be negative rather than ``gint``. +* Avoid using untyped pointers (e.g. gpointer) where practical.
Not that it matters, but I think `void` is technically an "incomplete type" rather than "untyped".
Since this rule is basically the same thing as the one above (presumably why it was placed here), I wonder if it would be better to just make it an example/extension on the previous rule?
ntrel commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
@codebrainz @elextr BTW I wrote the macros not to 'save a line or two of code' but to encapsulate iteration so when seeing the macro invocation you know there are no bugs in iteration (that was the intent anyway).
I accept the counter-argument that things like array iteration are simple and a custom macro is a geany-specific thing to learn, and it could possibly make debugging more confusing if the macro was passed a wrongly typed parameter.
But what about things that people often get wrong, like not checking `doc->is_valid`, I think the `foreach_document` macro is worth having. `is_valid` *is* a Geany-specific thing, so it's easy to justify a macro here. Sure, make it upper-case though.
@ntrel pushed 1 commit.
30e68ed5441b1cc89a7f81cdfd63fd82b1484e31 HACKING: merge 2 rules, simplify user_data clause
ntrel commented on this pull request.
@@ -219,6 +219,10 @@ Coding
``gint``, use a ``gchar`` for individual (ASCII/UTF-8) string characters rather than ``gint``, and use a ``guint`` for integers which cannot be negative rather than ``gint``. +* Avoid using untyped pointers (e.g. gpointer) where practical. +* Prefer loops to calling *_foreach() with a non-NULL untyped + ``user_data`` pointer parameter, unless external iteration is not + supported (e.g. for tree structures).
I've changed it to just "calling ``some_type_foreach()`` with a ``user_data`` argument". I think the last part implies that a NULL argument is OK. I agree that loops are generally better than *_foreach functions, but there are some cases where they're fine.
ntrel commented on this pull request.
@@ -219,6 +219,10 @@ Coding
``gint``, use a ``gchar`` for individual (ASCII/UTF-8) string characters rather than ``gint``, and use a ``guint`` for integers which cannot be negative rather than ``gint``. +* Avoid using untyped pointers (e.g. gpointer) where practical.
Good point, I've merged it with the above rule.
elextr commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
@ntrel, agree there is some argument for the document loop one, since there isn't any really obvious solution to the `is_valid` thing. (Aside, I thought the point of document IDs was to avoid dangling pointers, but my memory is a bit hazy and I don't have time just now to revise at this instant)
But if `FOREACH_DOCUMENT` is kept somehow it needs to be discoverable and documented, or people will only find it by accident reading existing code. For example what does it do if the set of documents is changed by the user? Or is that just documented "don't do that"?
But for the others, I agree with @codebrainz that since most contributors to Geany don't work on it constantly, using standard C is most likely to be understood.
codebrainz commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
But what about things that people often get wrong, like not checking doc->is_valid, I think the foreach_document macro is worth having. is_valid is a Geany-specific thing, so it's easy to justify a macro here. Sure, make it upper-case though.
If there's an actual reason for it, sure. But what is the reason we keep invalid documents around? If there is a good reason to do that (like not premature optimization), I would tend to agree such a macro may be useful, but probably located in the `document.h` file and as we all agree upper-case.
codebrainz commented on this pull request.
@@ -219,6 +219,10 @@ Coding
``gint``, use a ``gchar`` for individual (ASCII/UTF-8) string characters rather than ``gint``, and use a ``guint`` for integers which cannot be negative rather than ``gint``. +* Avoid using untyped pointers (e.g. gpointer) where practical. +* Prefer loops to calling *_foreach() with a non-NULL untyped + ``user_data`` pointer parameter, unless external iteration is not + supported (e.g. for tree structures).
...but there are some cases where they're fine
especially in languages with function expressions/lambdas :)
ntrel commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
it needs to be discoverable and documented, or people will only find it by accident reading existing code
Yes, that's why the `documents` macro has always mentioned it:
- @see documents_array(). */
#define documents ((GeanyDocument **)GEANY(documents_array)->pdata)
`documents_array` itself doesn't seem to be documented.
For example what does it do if the set of documents is changed by the user?
It's undefined and not documented. I think it's to be expected that iterator invalidation is not handled by the macro.
ntrel commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
what is the reason we keep invalid documents around? If there is a good reason to do that (like not premature optimization)
To avoid reading/writing to freed memory. The API allows people to read from a document pointer in a callback without seeing if they can find the document in documents_array. I don't think this can safely be deprecated, it would cause silent breakage.
but probably located in the document.h file
It already is.
Merged #2270 into master.
@ntrel DO NOT MERGE PRs while there are still discussions, I am tempted to revert on principle.
@elextr what was outstanding? I think the only unresolved comment thread was related but out-of-scope for this PR, no?
@codebrainz well, I didn't think anything was resolved, and neither of us nominated reviewers had approved.
codebrainz commented on this pull request.
@@ -2577,15 +2565,20 @@ void build_save_menu(GKeyFile *config, gpointer ptr, GeanyBuildSource src)
g_key_file_remove_key(config, build_grp_name, "error_regex", NULL); if (pj->priv->build_filetypes_list != NULL) { - data.config = config; - data.ft_names = g_ptr_array_new(); - g_ptr_array_foreach(pj->priv->build_filetypes_list, foreach_project_filetype, (gpointer)(&data)); - if (data.ft_names->pdata != NULL) + GPtrArray *ft_names = g_ptr_array_new(); + guint i; + + foreach_ptr_array(ft, i, pj->priv->build_filetypes_list)
Moved to a new issue #2281
Yeah, maybe we should either wait for one Github "approved" or else ping it before merging.
For this PR, I only did a superficial review of the code (hence no "approved"), but any specific issues I had were resolved and I agree with the changes in principle.
DO NOT MERGE PRs while there are still discussions
As Matthew said, the discussion was about foreach_, which the merged code no longer used.
neither of us nominated reviewers had approved.
It seemed Matthew agreed with the change because he suggested wording for the HACKING file. He hadn't commented on the build.c change, but as he appeared to support the principle in the HACKING file, the build.c change was enforcing the principle.
BTW, I'm not sure what you mean about 'nominated reviewers'?
I am tempted to revert on principle
You had told me a few months ago to leave pull requests open for at least a week, including a full weekend, before merging (I would still wait longer for any change likely to need discussion). I hadn't seen any new criteria for merging since then. Reverting should only be done if there's a commit that causes more harm than good, and the author is not able to fix it speedily.
Edit: Alternatively, we could say that if a PR has been submitted for over N days and has no Github "requested changes" on it, it's safe to merge.
Sounds good, but for simple changes N should be low. Sometimes there's a pull that causes merge conflicts on other pulls (not the case for this one though), then it's pragmatic to merge quite soon. Otherwise pulls are harder to review because they include dependent commits too.
It's good to usually merge stuff without waiting too long, particularly when follow-up work is likely. In master those changes will likely get tested more as more people will use them. If there's a problem, we can revert. I think this project has been too hesitant in the years I was away to merge code, work from developers can end up sitting in the review queue indefinitely. Maybe none of us will ever get around to reviewing it, despite the changes being very useful and working. In those cases, we should merge if the code is well written and the changes are wanted even if we won't test them ourselves, assuming the author says they've tested them adequately.
IOW, if you have an issue with a PR but no time to do a proper review, do a "requested changes" review with a short comment explaining as much.
I think a comment 'please wait for my review' should be enough, unless there are lots of other comments also.
ping it before merging
I agree with this if there have been no supportive comments.
It's good to usually merge stuff without waiting too long, particularly when follow-up work is likely. In master those changes will likely get tested more as more people will use them.
If there is a plan for a sequence of steps then that should be notified in an issue, possibly with a set of checkboxes listing the steps so they can be checked off as each PR on the process happens. But you can't expect people to read your mind about what your future plans for follow up are.
Those of us here with English as a first language need to remember that although we have the same language, we don't have the same technical, professional, experience or cultural backgrounds. So we may speak alike, but we do not think alike. That means we need to put more effort into communicating our background reasoning and future intentions and not assume others will understand automatically and will come to the same conclusion about how a specific PR should be approached.
For instance, concern has been expressed that some PRs are [rearranging the deckchairs on the titanic](https://en.wiktionary.org/wiki/rearrange_the_deck_chairs_on_the_Titanic) and not achieving any substantive improvement. Whilst I defend your right to align deckchairs, it would be good to know if their is a master plan behind it or just an itchy spot to scratch.
github-comments@lists.geany.org