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 all #if 0...#endif and some other dead code #913

Merged
merged 1 commit into from
Apr 16, 2020

Conversation

alerque
Copy link
Contributor

@alerque alerque commented Aug 14, 2018

This should be a complete no-op code wise. Nothing in this commit should
change the function of the program in any way. If git bisect or
similar ever traces a problem to this commit, then something is wrong
and I didn't match start/end markers properly.

This partially addresses issue #894, bullet point 1 & part of 2.

Copy link
Contributor

@yetist yetist left a comment

Choose a reason for hiding this comment

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

In the quick review, I think there is no problem with this change.

@alerque
Copy link
Contributor Author

alerque commented Dec 20, 2019

I just forced-pushed an amended commit to poke the CI builds which were not working upstream at the time I submitted this. Exactly 0 code was changed between commits, and this still merges cleanly to master.

@alerque
Copy link
Contributor Author

alerque commented Dec 20, 2019

@greg-hellings Continuing conversation from #965 here (see my objection to the differences in the duplicate PR).

The last expression I saw from @karlkleinpaste suggested that he might be warming up to this idea.

perhaps it's time to make them go away.

: imodule.renderText().c_str()));
# else
// gosh this is gross.
// begin gosh this is gross.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why you do this unrelated changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It isn't unrelated. Without the wrapping #if 0 ... else ... end block that got removed, it was no longer clear what section of code the remaining comment referred to. I defined the region to not loose the little bit of information provided by the fact that this code was previously grouped together.

#endif /* !0 */

// end grossness

Copy link
Contributor

Choose a reason for hiding this comment

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

Why you do the unrelated changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not unrelated. See this explanation.

This should be a complete no-op code wise. Nothing in this commit should
change the function of the program in any way. If `git bisect` or
similar ever traces a problem to this commit, then something is wrong
and I didn't match start/end markers properly.

This partially addresses issue crosswire#894, bullet point 1 & part of 2.
@alerque
Copy link
Contributor Author

alerque commented Mar 26, 2020

Ping. @greg-hellings or whoever, do we have any progress on this? Every time I open up the code to review something I bump into these dead blocks and get frustrated. It's really a lot easier on contributors if dead code is left in git history instead of in the files (and if PRs get reviewed and merged within at least a few years).

@greg-hellings
Copy link
Contributor

@alerque - I'm perfectly comfortable with this being merged, but when I had made the same proposal many years ago @karlkleinpaste had expressed his desire to keep this code in place rather than in git history. I would rather not hit the Merge button over his wishes without hearing from him directly that it's OK. If he wants this merged, all he needs to do is click the "Merge pull request" button, so if he hasn't then I don't feel right going over his choice. He's the project lead.

Copy link
Contributor

@greg-hellings greg-hellings left a comment

Choose a reason for hiding this comment

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

I strongly approve of this sort of code update! @karlkleinpaste, please consider merging this.

@karlkleinpaste
Copy link
Contributor

i'll just mention that i'm about to merge this, but FYI i'm going to be doing a little more beyond this to get rid of the "gosh this is gross" section specifically because of more recent understanding about self-closing div and get rid of the gross hack entirely.

as for keeping around old #if 0 blocks... in my defense, it was a habit learned of much brutal experience literally decades in the past, and prior to the advances of things like git. it no longer matters as much to me as it once did.

@karlkleinpaste karlkleinpaste merged commit 8c58e53 into crosswire:master Apr 16, 2020
@alerque
Copy link
Contributor Author

alerque commented Apr 16, 2020

Thanks @karlkleinpaste. I cut my teeth writing code before versions control too. I have no idea how I survived those dreadful years.

@alerque alerque deleted the cleanup branch April 16, 2020 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants