cleanup: Remove implicit bool conversions. #2621

iphydf · 2024-01-31T19:22:01Z

This change is

codecov · 2024-01-31T19:34:02Z

Codecov Report

Attention: 16 lines in your changes are missing coverage. Please review.

Comparison is base (4359e3a) 73.71% compared to head (b7404f2) 73.57%.

Files	Patch %	Lines
toxcore/TCP_connection.c	41.17%	10 Missing ⚠️
toxcore/DHT.c	90.90%	2 Missing ⚠️
toxcore/friend_connection.c	33.33%	2 Missing ⚠️
toxav/msi.c	66.66%	1 Missing ⚠️
toxcore/Messenger.c	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2621      +/-   ##
==========================================
- Coverage   73.71%   73.57%   -0.15%     
==========================================
  Files         148      148              
  Lines       30424    30420       -4     
==========================================
- Hits        22428    22382      -46     
- Misses       7996     8038      +42

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

JFreegman

Reviewed 19 of 19 files at r1, all commit messages.
Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf)

toxcore/group_chats.c line 3918 at r1 (raw file):

        // the topic version should never change when the topic lock is disabled except when
        // the founder changes the topic prior to enabling the lock
        if (peer->role != GR_FOUNDER || topic_info->version != chat->shared_state.topic_lock + 1) {

Why is this being changed? While I can see people having a preference for one or the other, personally I find that the former logic is easier to parse.

toxcore/group_moderation.c line 553 at r1 (raw file):

    if (moderation->shared_state_version > 0) {
        if ((creds->version < moderation->sanctions_creds.version)
                && (creds->version != 0 || moderation->sanctions_creds.version != UINT32_MAX)) {

Same question as above.

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

                /* When switching from TCP to UDP, don't change the packet send rate for CONGESTION_EVENT_TIMEOUT ms. */
                if (!(direct_connected && conn->last_tcp_sent + CONGESTION_EVENT_TIMEOUT > temp_time)) {

Same as above

toxcore/onion_client.c line 1936 at r1 (raw file):

        && (paths->last_path_used_times[pathnum] == 0
            || !mono_time_is_timeout(
                mono_time, paths->last_path_used[pathnum], ONION_PATH_TIMEOUT));

This entire clause is insane. We should think about refactoring it.

nurupo

Looked only over the bootstrap daemon changes. Some changes I'm happy with, e.g. the functions returning 0/1 changing to return false/true is a very welcome change, but some I'm not happy with, like if (!pointer) -> if (pointer == nullptr) and some others.

iphydf

Reviewable status: 0 of 1 approvals obtained (waiting on @JFreegman)

toxcore/group_chats.c line 3918 at r1 (raw file):

Previously, JFreegman wrote…

Why is this being changed? While I can see people having a preference for one or the other, personally I find that the former logic easier to parse.

Done.

toxcore/group_moderation.c line 553 at r1 (raw file):

Previously, JFreegman wrote…

Same question as above.

Done.

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

Previously, JFreegman wrote…

Same as above

Done.

toxcore/onion_client.c line 1936 at r1 (raw file):

Previously, JFreegman wrote…

This entire clause is insane. We should think about refactoring it.

I looked at it a bit. That raised more questions than I want to resolve for this PR.

iphydf · 2024-01-31T22:28:41Z

Looked only over the bootstrap daemon changes. Some changes I'm happy with, e.g. the functions returning 0/1 changing to return false/true is a very welcome change, but some I'm not happy with, like if (!pointer) -> if (pointer == nullptr) and some others.

Please mark the ones you're not happy with and want reverted.

The !pointer change is necessary if we ever want to have a stronger type system enforcement on this code. We do this everywhere in toxcore except in the bootstrap node, because pointers aren't booleans, and implicit conversions are fundamentally tricky. If we can completely eliminate those, at least in some kind of overlay type system, reasoning about what type a value has becomes much more straightforward. E.g. in if (forwarding) {...} it's entirely unclear whether the branch is about whether we want forwarding or not (bool), whether there's at least 1 forwarding (uint), whether forwarding failed (int return from a posix function non-zero), an allocation failed (pointer), or some other variety of truthiness.

nurupo

Reviewed 9 of 19 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf and @JFreegman)

other/bootstrap_daemon/src/config.c line 407 at r2 (raw file):

        free(bs_public_key_bin);

        if (address_resolved == 0) {

This was more readable before.

How about the following?

bool address_resolved;
...
address_resolved = dht_bootstrap_from_address(dht, bs_address, enable_ipv6, net_htons(bs_port), bs_public_key_bin) == 0;
...
if (!address_resolved) {

I would prefer to not have this == 0 and let do the int-to-bool conversion do the trick, but I take it you want an explicit conversion.

toxcore/onion_client.c line 1928 at r2 (raw file):

non_null()
static bool path_is_stable(const Mono_Time *mono_time, const Onion_Client_Paths *paths,
    uint32_t pathnum, const Onion_Node *node)

Pad the arguments with spaces to match where they start on the 1st line?

JFreegman

Reviewed 10 of 10 files at r2, all commit messages.
Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @iphydf)

iphydf

Reviewable status: complete! 1 change requests, 1 of 1 approvals obtained (waiting on @JFreegman and @nurupo)

other/bootstrap_daemon/src/config.c line 407 at r2 (raw file):

Previously, nurupo wrote…

This was more readable before.

How about the following?
bool address_resolved;
...
address_resolved = dht_bootstrap_from_address(dht, bs_address, enable_ipv6, net_htons(bs_port), bs_public_key_bin) == 0;
...
if (!address_resolved) {
I would prefer to not have this == 0 and let do the int-to-bool conversion do the trick, but I take it you want an explicit conversion.

Changed dht_bootstrap_from_address to return bool, instead.

toxcore/onion_client.c line 1928 at r2 (raw file):

Previously, nurupo wrote…

Pad the arguments with spaces to match where they start on the 1st line?

Tell that to astyle and/or clang-format.

Restyled astyle is fixed now.

nurupo · 2024-02-02T04:15:05Z

Somehow I can't review just the first commit in Reviewable, in "Review each commit separately" mode each commit should be a separate revision, yet the DHT.h changes somehow got lumped together with the formatting changes into r3 :\

nurupo

Reviewable status: complete! 2 of 1 approvals obtained (waiting on @iphydf and @JFreegman)

toxcore/onion_client.c line 1928 at r2 (raw file):

Previously, iphydf wrote…

Tell that to astyle and/or clang-format.

I noticed the other function in that file were doing that, so pointed it out.

Looks like this has been fixed now that the code was astyled.

nurupo · 2024-02-02T04:23:29Z

Somehow I can't review just the first commit in Reviewable, in "Review each commit separately" mode each commit should be a separate revision, yet the DHT.h changes somehow got lumped together with the formatting changes into r3 :\

I guess the "Review each commit separately" mode just doesn't work, it's still in the "combine commits" mode, and it put both the code formatting 1st commit + the updated 2nd commit as rev3, so I'm unable to see what you changed since my last review as the 1st commit clutters all of the changes I'm looking for -- I want to ignore the 1st commit and see only 2nd commit changes since the last time. So annoying. Reviewable is great when it works, but annoying when it doesn't. Had to manually re-review the entire 2nd commit.

iphydf added this to the v0.2.19 milestone Jan 31, 2024

iphydf marked this pull request as ready for review January 31, 2024 19:22

iphydf force-pushed the bools branch 2 times, most recently from 698c4c1 to 0185de1 Compare January 31, 2024 19:25

JFreegman reviewed Jan 31, 2024

View reviewed changes

pull-request-attention bot assigned iphydf Jan 31, 2024

nurupo reviewed Jan 31, 2024

View reviewed changes

iphydf force-pushed the bools branch from 0185de1 to f1a5a1f Compare January 31, 2024 20:39

iphydf commented Jan 31, 2024

View reviewed changes

iphydf force-pushed the bools branch from f1a5a1f to 414d7c7 Compare January 31, 2024 20:40

iphydf force-pushed the bools branch from 414d7c7 to bc182e4 Compare January 31, 2024 22:36

nurupo requested a review from JFreegman February 1, 2024 01:27

nurupo requested changes Feb 1, 2024

View reviewed changes

pull-request-attention bot assigned JFreegman Feb 1, 2024

JFreegman approved these changes Feb 1, 2024

View reviewed changes

pull-request-attention bot unassigned JFreegman Feb 1, 2024

iphydf commented Feb 2, 2024

View reviewed changes

iphydf force-pushed the bools branch from bc182e4 to f634732 Compare February 2, 2024 01:13

restyled-io bot mentioned this pull request Feb 2, 2024

Restyle cleanup: Remove implicit bool conversions. #2631

Closed

iphydf force-pushed the bools branch 2 times, most recently from c765017 to 6303227 Compare February 2, 2024 01:18

iphydf added 2 commits February 2, 2024 01:31

chore: Reformat sources with astyle.

4e2dba4

Restyled astyle is fixed now.

cleanup: Remove implicit bool conversions.

b7404f2

iphydf force-pushed the bools branch from 6303227 to b7404f2 Compare February 2, 2024 01:31

nurupo requested a review from JFreegman February 2, 2024 04:17

nurupo approved these changes Feb 2, 2024

View reviewed changes

pull-request-attention bot assigned JFreegman Feb 2, 2024

toktok-releaser merged commit b7404f2 into TokTok:master Feb 2, 2024
59 checks passed

iphydf mentioned this pull request Feb 2, 2024

chore: Reformat sources with astyle. #2629

Merged

nurupo mentioned this pull request Feb 17, 2024

refactor: Make tox-bootstrapd use bool instead of int #2692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup: Remove implicit bool conversions. #2621

cleanup: Remove implicit bool conversions. #2621

iphydf commented Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

JFreegman left a comment •

edited

Loading

nurupo left a comment •

edited

Loading

iphydf left a comment

iphydf commented Jan 31, 2024

nurupo left a comment

JFreegman left a comment

iphydf left a comment

nurupo commented Feb 2, 2024

nurupo left a comment

nurupo commented Feb 2, 2024 •

edited

Loading

cleanup: Remove implicit bool conversions. #2621

cleanup: Remove implicit bool conversions. #2621

Conversation

iphydf commented Jan 31, 2024 • edited Loading

codecov bot commented Jan 31, 2024 • edited Loading

Codecov Report

JFreegman left a comment • edited Loading

Choose a reason for hiding this comment

nurupo left a comment • edited Loading

Choose a reason for hiding this comment

iphydf left a comment

Choose a reason for hiding this comment

iphydf commented Jan 31, 2024

nurupo left a comment

Choose a reason for hiding this comment

JFreegman left a comment

Choose a reason for hiding this comment

iphydf left a comment

Choose a reason for hiding this comment

nurupo commented Feb 2, 2024

nurupo left a comment

Choose a reason for hiding this comment

nurupo commented Feb 2, 2024 • edited Loading

iphydf commented Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

JFreegman left a comment •

edited

Loading

nurupo left a comment •

edited

Loading

nurupo commented Feb 2, 2024 •

edited

Loading