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

Using stdint instead of int/long #1009

Merged
merged 2 commits into from
Jul 18, 2018
Merged

Using stdint instead of int/long #1009

merged 2 commits into from
Jul 18, 2018

Conversation

hugbubby
Copy link
Member

@hugbubby hugbubby commented Jul 16, 2018

Attempting to use fixed-sized integers as per #974. Only other changes were a todo comment and a functionally nonexistent modification to two for loops.


This change is Reviewable

@hugbubby hugbubby force-pushed the master branch 2 times, most recently from 9c78536 to b5bd7e2 Compare July 16, 2018 01:23
Copy link
Member

@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


auto_tests/conference_test.c, line 200 at r1 (raw file):

    }

    printf("friends connected, took %d seconds\n", (uint16_t)(time(nullptr) - cur_time));

%d requires you to pass an int, exactly that type, no other type. unsigned short happens to be promoted to int when passed through varargs, but that's just a pointless conversion/truncation you're doing here. In formatting statements, don't do fixed-size int casts. In all other code, yes.

Copy link
Member

@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


auto_tests/conference_two_test.c, line 23 at r1 (raw file):

    TOX_ERR_CONFERENCE_NEW err;
    tox_conference_new(tox1, &err);
    ck_assert_msg(err == TOX_ERR_CONFERENCE_NEW_OK, "Failed to create conference 1: %d.", err);

Error messages should start with lowercase and not have a "." at the end, so that they can easily be composed into sentences by error message wrappers.

Copy link
Member

@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


auto_tests/conference_two_test.c, line 23 at r1 (raw file):

Previously, iphydf wrote…

Error messages should start with lowercase and not have a "." at the end, so that they can easily be composed into sentences by error message wrappers.

We're using this style rule and justification: https://github.com/golang/go/wiki/CodeReviewComments#error-strings

@iphydf iphydf modified the milestones: v0.2.x, v0.2.4 Jul 16, 2018
@hugbubby
Copy link
Member Author

hugbubby commented Jul 17, 2018

You know, if we want, there is a way to use fixed-width integers in print statements: https://en.cppreference.com/w/cpp/types/integer

printf("%"PRIu16"\n", (uint16_t) size_variable)

might be a bit much though. What do you think @iphydf?

@iphydf
Copy link
Member

iphydf commented Jul 17, 2018

I think that does not improve readability, so I'd rather avoid those PRI macros.

@hugbubby hugbubby force-pushed the master branch 2 times, most recently from 566d86e to 6f813ae Compare July 18, 2018 16:47
@hugbubby
Copy link
Member Author

I did a git rebase and then a git push --force. I think that was correct. Not sure though.

Copy link
Member

@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.

:lgtm_strong:

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

Did my best to surmise the size requirements of
these integers, will do the rest of the tests soon. Also added a todo
and made an obsessive change to a for loop.
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