Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build.c: Remove g_ptr_array_foreach with gpointer user_data & update HACKING #2270

Merged
merged 4 commits into from Aug 29, 2019

Conversation

@ntrel
Copy link
Member

commented Aug 22, 2019

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

static void foreach_project_filetype(gpointer data, gpointer user_data)
{
GeanyFiletype *ft = data;
ForEachData *d = user_data;

This comment has been minimized.

Copy link
@ntrel

ntrel Aug 22, 2019

Author Member

C really should require a cast for these to highlight the unsafety.

src/build.c Outdated Show resolved Hide resolved
HACKING Outdated Show resolved Hide resolved
HACKING Outdated Show resolved Hide resolved

@ntrel ntrel merged commit 0ce5d54 into geany:master Aug 29, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ntrel ntrel deleted the ntrel:foreach-user-data branch Aug 29, 2019

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 29, 2019

@ntrel DO NOT MERGE PRs while there are still discussions, I am tempted to revert on principle.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@elextr what was outstanding? I think the only unresolved comment thread was related but out-of-scope for this PR, no?

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

@codebrainz well, I didn't think anything was resolved, and neither of us nominated reviewers had approved.

@codebrainz

This comment has been minimized.

Copy link
Member

commented Aug 30, 2019

Yeah, maybe we should either wait for one Github "approved" or else ping it before merging.

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

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.

@ntrel

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

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.

@ntrel

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

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.

@ntrel

This comment has been minimized.

Copy link
Member Author

commented Aug 30, 2019

ping it before merging

I agree with this if there have been no supportive comments.

@elextr

This comment has been minimized.

Copy link
Member

commented Aug 31, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.