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

Remove useless lines caused by declaration-after-statement warning #266

Merged
merged 2 commits into from
Dec 7, 2020

Conversation

JelteF
Copy link
Contributor

@JelteF JelteF commented May 13, 2020

This is done using the same automated technique introduced for the Citus
repo in citusdata/citus#3181. The main idea is
to replace:

int a = 0;
// some code
a = 1;

with:

// some code
int a = 1;

For details of how this works see the above mentioned Citus PR.

This comment about not doing early an early return to limit the scope of
variables caused me to add this here too:
#244 (comment)
With this commit merged, scope of variables will always be minimal in
cases where an early return is possible.

To make sure merge conflict handling is easy the following steps should be done:

  1. Before merging this, all open PRs should be rebased on master.
  2. Then this can be merged.
  3. Then all PRs should be rebased on master again.
  4. If merge conflicts occur (which is likely), they can be resolved trivially
    by doing git checkout --theirs src to ignore the changes caused by this PR.
    Then running ci/remove_useless_declarations.sh will redo all the changes.

To further limit impact, this PR can be merged right before a release, to make
sure most open PRs have been merged. This PR can be trivially updated as well by doing:

git rebase
# if merge conflict run
git checkout --ours src

@JelteF JelteF force-pushed the allow-declaration-after-statement branch from da921aa to 66d28ed Compare May 13, 2020 10:20
@JelteF JelteF requested a review from DimCitus May 13, 2020 10:37
@JelteF JelteF self-assigned this May 13, 2020
@JelteF JelteF added this to the pg_auto_failover 1.4 milestone May 13, 2020
@JelteF JelteF force-pushed the allow-declaration-after-statement branch from e8d06d4 to 4db0ccc Compare May 13, 2020 10:50
@JelteF JelteF force-pushed the allow-declaration-after-statement branch 4 times, most recently from f0954cc to 1ad2d02 Compare December 7, 2020 13:17
Copy link
Collaborator

@DimCitus DimCitus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be best if src/tools/remove_useless_declarations.sh would just work from the top-level directory, so we might want to either add a Makefile target or use git rev-parse --show-toplevel or such so that the script always work, whatever the current working directory happens to be when it's invoked.

Otherwise we need to remember more things about it, and we just won't use it, and when we want to it won't work.

LGTM otherwise: as mentioned in conversation, many of the changes in the PR are not very useful, and I'm quite neutral about it. That said, some of the changes are net improvements, and I don't feel like blocking the PR because of the neutral changes: they are neutral after all.

This is done using the same automated technique introduced for the Citus
repo in citusdata/citus#3181. The main idea is
to replace:

```c
int a = 0;
// some code
a = 1;
```

with:

```c
// some code
int a = 1;
```

For details of how this works see the above mentioned Citus PR.

This comment about not doing early an early return to limit the scope of
variables caused me to add this here too:
#244 (comment)
With this commit merged, scope of variables will always be minimal in
cases where an early return is possible.
@JelteF JelteF force-pushed the allow-declaration-after-statement branch from 1ad2d02 to eba135e Compare December 7, 2020 13:33
@JelteF JelteF merged commit a7ae375 into master Dec 7, 2020
@JelteF JelteF deleted the allow-declaration-after-statement branch December 7, 2020 14:20
@DimCitus DimCitus added Developer productivity Enhancements to ability to ship quality code enhancement New feature or request Size:M Effort Estimate: Medium labels Dec 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer productivity Enhancements to ability to ship quality code enhancement New feature or request Size:M Effort Estimate: Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants