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

Add missing #includes to headers and rename tox_old to tox_group. #74

Merged
merged 1 commit into from
Aug 31, 2016

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Aug 30, 2016

Also, no longer #include the group code into tox.c. Instead, compile it
separately in tox_group.c. This is a bit less surprising to someone looking
around the code. Having some implementations in a .h file is certainly a bit
surprising to a disciplined C programmer, especially when there is no technical
reason to do it.


This change is Reviewable

@@ -1,3 +1,8 @@
#ifndef TOX_OLD_H
#define TOX_OLD_H
Copy link
Member

Choose a reason for hiding this comment

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

TOX_GROUP_H

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Also, no longer #include the group code into tox.c. Instead, compile it
separately in tox_group.c. This is a bit less surprising to someone looking
around the code. Having some implementations in a .h file is certainly a bit
surprising to a disciplined C programmer, especially when there is no technical
reason to do it.
@nurupo
Copy link
Member

nurupo commented Aug 31, 2016

:lgtm:


Reviewed 10 of 11 files at r1, 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@iphydf iphydf closed this Aug 31, 2016
@iphydf iphydf deleted the includes branch August 31, 2016 16:05
@iphydf iphydf merged commit fa3b512 into TokTok:master Aug 31, 2016
@iphydf iphydf modified the milestone: v0.0.1 Nov 6, 2016
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.

2 participants