-
Notifications
You must be signed in to change notification settings - Fork 52
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
Conversation
There was a problem hiding this 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.
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. |
@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.
|
: imodule.renderText().c_str())); | ||
# else | ||
// gosh this is gross. | ||
// begin gosh this is gross. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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). |
@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. |
There was a problem hiding this 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.
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. |
Thanks @karlkleinpaste. I cut my teeth writing code before versions control too. I have no idea how I survived those dreadful years. |
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
orsimilar 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.