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

Group #include directives in 3-4 groups. #125

Merged
merged 1 commit into from
Sep 13, 2016
Merged

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Sep 11, 2016

  1. Current module (if C file).
  2. Headers from current library.
  3. Headers from other library (e.g. toxcore includes in toxav).
  4. System headers.

This change is Reviewable

@GrayHatter
Copy link

Reviewed 38 of 38 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toxav/toxav_old.c, line 25 at r1 (raw file):

 */

#include "toxav.h"

This section is audio support for groupchats. Is this a groupchat first module, adding audio support? Or is it an toxav module incorporating groupchat functions?


toxcore/assoc.c, line 7 at r1 (raw file):

#include "assoc.h"

#include "DHT.h"

Assoc is a superset of DHT, would DHT.c fit better in the current module group?


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Sep 11, 2016

Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toxav/toxav_old.c, line 25 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

This section is audio support for groupchats. Is this a groupchat first module, adding audio support? Or is it an toxav module incorporating groupchat functions?

It should be toxav_group.c and toxav_group.h. I didn't change that in this PR to avoid unrelated changes. This PR is about include reordering.

toxcore/assoc.c, line 7 at r1 (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Assoc is a superset of DHT, would DHT.c fit better in the current module group?

The rule is: $module.c includes $module.h as first thing. Everything else comes in blocks after it. Other than config.h, no exceptions.

Comments from Reviewable

@GrayHatter
Copy link

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 12, 2016

Shouldn't this header inclusion policy be documented somewhere?


Reviewed 4 of 38 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@nurupo
Copy link
Member

nurupo commented Sep 12, 2016

Reviewed 34 of 38 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


toxcore/net_crypto.c, line 33 at r1 (raw file):

#include "math.h"
#include "util.h"

Shouldn't util.h be before math.h? Also it should be <math.h>


toxcore/tox.c, line 32 at r1 (raw file):

#include "tox.h"

#include "../toxencryptsave/defines.h"

That's a header from another library, should be moved 1 block down.


Comments from Reviewable

@iphydf
Copy link
Member Author

iphydf commented Sep 12, 2016

Yes, we should have all our policies documented. I'll do that in another PR at some point when I feel creative (or you can do it :).


Review status: 36 of 38 files reviewed at latest revision, 2 unresolved discussions.


toxcore/net_crypto.c, line 33 at r1 (raw file):

Previously, nurupo wrote…

Shouldn't util.h be before math.h? Also it should be <math.h>

Yep. Didn't catch that one because it was "".

toxcore/tox.c, line 32 at r1 (raw file):

Previously, nurupo wrote…

That's a header from another library, should be moved 1 block down.

Done.

Comments from Reviewable

@iphydf iphydf force-pushed the header-order branch 2 times, most recently from 2586db9 to 3a45e3d Compare September 12, 2016 20:42
@nurupo
Copy link
Member

nurupo commented Sep 12, 2016

:lgtm_strong:


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

1. Current module (if C file).
2. Headers from current library.
3. Headers from other library (e.g. toxcore includes in toxav).
4. System headers.
@iphydf iphydf closed this Sep 13, 2016
@iphydf iphydf deleted the header-order branch September 13, 2016 00:05
@iphydf iphydf merged commit 0aa2840 into TokTok:master Sep 13, 2016
@iphydf iphydf modified the milestone: v0.0.1 Nov 5, 2016
This pull request was closed.
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.

3 participants