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

Style fixes in TCP code; remove MIN and PAIR from util.h. #997

Merged
merged 1 commit into from
Jul 12, 2018

Conversation

iphydf
Copy link
Member

@iphydf iphydf commented Jul 12, 2018

  • Moved PAIR to toxav, where it's used (but really this should die).
  • Replace most MIN calls with typed min_* calls. Didn't replace the
    ones where the desired semantics are unclear. Moved the MIN macro to
    the one place where it's still used.
  • Avoid assignments in while loops. Instead, factored out the loop body
    into a separate bool-returning function.
  • Use named types for callbacks (_cb types).
  • Avoid assignments in if conditions.
  • Removed MAKE_REALLOC and expanded its two calls. We can't have
    templates in C, and this fake templating is ugly and hard to analyse
    and debug (it expands on a single line).
  • Moved epoll system include to the .c file, out of the .h file.
  • Avoid assignments in expressions (a = b = c;).
  • Avoid multiple declarators per struct member declaration.
  • Fix naming inconsistencies.
  • Replace net_to_host macro with function.

This change is Reviewable

Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 19 files at r1.
Reviewable status: 0 of 1 LGTMs obtained (waiting on @sudden6)


toxcore/network.c, line 1384 at r1 (raw file):

    // Used to avoid malloc parameter overflow
    const size_t MAX_COUNT = min_u64(SIZE_MAX, INT32_MAX) / sizeof(IP_Port);

is SIZE_MAX guaranteed to always fit in a uint64_t? If not, at least add a comment.


toxcore/TCP_client.c, line 38 at r1 (raw file):

typedef struct TCP_Client_Conn {
    uint8_t status; /* 0 if not used, 1 if other is offline, 2 if other is online. */

Turn this into an enum? If not in this PR maybe add a TODO?


toxcore/TCP_server.c, line 55 at r1 (raw file):

    uint8_t public_key[CRYPTO_PUBLIC_KEY_SIZE];
    uint32_t index;
    uint8_t status; /* 0 if not used, 1 if other is offline, 2 if other is online. */

enum?

@iphydf iphydf force-pushed the tcp-named-cb branch 2 times, most recently from 6140d3a to ec37ccf Compare July 12, 2018 19:57
Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained (waiting on @sudden6)


toxcore/network.c, line 1384 at r1 (raw file):

Previously, sudden6 wrote…

is SIZE_MAX guaranteed to always fit in a uint64_t? If not, at least add a comment.

Yes, the only platform that this would potentially not fit into would be one with 128 bit memory addressing, and toxcore doesn't support any of those. The only architectures that have 128 bit or greater memory addressing are GPUs and even there size_t would be at most 64 bit (the wider bus allows parallelism, not more memory).


toxcore/TCP_client.c, line 38 at r1 (raw file):

Previously, sudden6 wrote…

Turn this into an enum? If not in this PR maybe add a TODO?

Done.


toxcore/TCP_server.c, line 55 at r1 (raw file):

Previously, sudden6 wrote…

enum?

Done.

@iphydf iphydf force-pushed the tcp-named-cb branch 2 times, most recently from 02d5249 to 287416f Compare July 12, 2018 20:00
Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 19 files at r1, 6 of 7 files at r2.
Reviewable status: 0 of 1 LGTMs obtained (waiting on @sudden6)


toxcore/network.c, line 1384 at r1 (raw file):

Previously, iphydf wrote…

Yes, the only platform that this would potentially not fit into would be one with 128 bit memory addressing, and toxcore doesn't support any of those. The only architectures that have 128 bit or greater memory addressing are GPUs and even there size_t would be at most 64 bit (the wider bus allows parallelism, not more memory).

To prepare for the future and exotic machines add an assert?

* Moved PAIR to toxav, where it's used (but really this should die).
* Replace most MIN calls with typed `min_*` calls. Didn't replace the
  ones where the desired semantics are unclear. Moved the MIN macro to
  the one place where it's still used.
* Avoid assignments in `while` loops. Instead, factored out the loop body
  into a separate `bool`-returning function.
* Use named types for callbacks (`_cb` types).
* Avoid assignments in `if` conditions.
* Removed `MAKE_REALLOC` and expanded its two calls. We can't have
  templates in C, and this fake templating is ugly and hard to analyse
  and debug (it expands on a single line).
* Moved epoll system include to the .c file, out of the .h file.
* Avoid assignments in expressions (`a = b = c;`).
* Avoid multiple declarators per struct member declaration.
* Fix naming inconsistencies.
* Replace `net_to_host` macro with function.
Copy link
Member Author

@iphydf iphydf left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained (waiting on @sudden6)


toxcore/network.c, line 1384 at r1 (raw file):

Previously, sudden6 wrote…

To prepare for the future and exotic machines add an assert?

Done, in monolith_test.c, as static assert.

Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r2, 4 of 4 files at r3.
Reviewable status: 0 of 1 LGTMs obtained (waiting on @sudden6)

Copy link

@sudden6 sudden6 left a comment

Choose a reason for hiding this comment

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

:lgtm_strong:

Reviewable status: :shipit: complete! 1 of 1 LGTMs obtained

@iphydf iphydf merged commit beeb9b4 into TokTok:master Jul 12, 2018
@iphydf iphydf deleted the tcp-named-cb branch July 12, 2018 21:23
@iphydf iphydf modified the milestones: v0.2.x, v0.2.4 Jul 16, 2018
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.

None yet

2 participants