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

cleanup: Make group packet entry creation less error-prone #2540

Merged
merged 1 commit into from
Jan 11, 2024

Conversation

JFreegman
Copy link
Member

@JFreegman JFreegman commented Jan 10, 2024

We always assumed that create_array_entry() would only be called with an empty array entry and wouldn't modify entries on error. We now explicitly require both conditions, and also give an error in the case of a non-null data pointer with a zero length field, as this indicates a logic error.

Checks for an empty array entry that precede a call to create_array_entry() are now redundant. It should be noted that a non-empty entry doesn't necessarily indicate an error. This condition can be triggered if packets are being sent or received faster than they can be processed/acknowledged, which is common when spamming messages on poor connections.


This change is Reviewable

@JFreegman JFreegman added the cleanup Internal code cleanup, possibly affecting semantics, e.g. deleting a deprecated feature. label Jan 10, 2024
@JFreegman JFreegman added this to the v0.2.19 milestone Jan 10, 2024
@JFreegman JFreegman requested review from nurupo and iphydf January 10, 2024 16:54
@JFreegman JFreegman force-pushed the group_array_entry_cleanup branch from 2cc38dc to 515e143 Compare January 10, 2024 17:08
@JFreegman
Copy link
Member Author

This cleanup found a bug, which is fixed in #2541

@JFreegman JFreegman force-pushed the group_array_entry_cleanup branch 2 times, most recently from 4db8e90 to 969f514 Compare January 10, 2024 17:54
Copy link

codecov bot commented Jan 10, 2024

Codecov Report

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

Comparison is base (5b9c420) 69.08% compared to head (6b6718e) 69.02%.

Files Patch % Lines
toxcore/group_connection.c 50.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2540      +/-   ##
==========================================
- Coverage   69.08%   69.02%   -0.07%     
==========================================
  Files          96       96              
  Lines       28052    28049       -3     
==========================================
- Hits        19381    19362      -19     
- Misses       8671     8687      +16     

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

@JFreegman JFreegman force-pushed the group_array_entry_cleanup branch from 969f514 to 7d402bd Compare January 10, 2024 20:45
@JFreegman JFreegman force-pushed the group_array_entry_cleanup branch from 7d402bd to 2be6535 Compare January 11, 2024 05:34
Copy link
Member

@nurupo nurupo left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @iphydf and @JFreegman)


-- commits line 15 at r2:
What width do you use for the commit message body? Seems to be shorter than the typical 72 columns. Looks like 64 or 65?

@JFreegman
Copy link
Member Author

-- commits line 15 at r2:

Previously, nurupo wrote…

What width do you use for the commit message body? Seems to be shorter than the typical 72 columns. Looks like 64 or 65?

I just eyeball it. Is there a way to force commit messages to print a newline at a specific column via terminal?

@JFreegman JFreegman force-pushed the group_array_entry_cleanup branch from 0dc6fd9 to 4d703a4 Compare January 11, 2024 15:55
@nurupo
Copy link
Member

nurupo commented Jan 11, 2024

-- commits line 15 at r2:

Previously, JFreegman wrote…

I just eyeball it. Is there a way to force commit messages to print a newline at a specific column via terminal?

You could check the line wrapping functionality of your terminal text editor.

If you are using nano, you can add set fill 0 into your .nanorc file, which will make it wrap using the screen width, so if you resize your terminal window to be 72 characters long (some terminal emulators show you the new size while you resize the window so it's very easy to tell) and then press Ctrl+j while having a cursor in a paragraph to wrap (justify) just that paragraph, or Alt+j to wrap the entire buffer -- that would wrap the text at 72 characters on whitespace, Of course you could just do set fill 72 and then it would wrap to 72 no matter the terminal window size, but I like the flexibility of 0.

You can also get your commit message spellchecked by adding set speller "aspell -x -c" to .nanorc and pressing F12 to spellcheck. (I'm actually having nano invoke a wrapper bash script instead of invoking aspell directly, which keeps a personal dictionary file per git repository where aspell adds per-project words, to avoid aspell triggering on a ton of technical jargon used in a git repository that is not part of the English dictionary.)

We always assumed that create_array_entry() would only be called
with an empty array entry and wouldn't modify entries on error.
We now explicitly require both conditions, and also give an
error in the case of a non-null data pointer with a zero
length field, as this indicates a logic error.

Checks for an empty array entry that precede a call to
create_array_entry() are now redundant. It should be noted that
a non-empty entry doesn't necessarily indicate an error. This
condition can be triggered if packets are being sent or
received faster than they can be processed/acknowledged,
which is common when spamming messages on poor connections.
@JFreegman JFreegman force-pushed the group_array_entry_cleanup branch from 4d703a4 to 6b6718e Compare January 11, 2024 16:02
@JFreegman JFreegman removed the request for review from iphydf January 11, 2024 16:17
Copy link
Member

@Green-Sky Green-Sky 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 2 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @JFreegman)

@JFreegman JFreegman merged commit 6b6718e into TokTok:master Jan 11, 2024
54 of 55 checks passed
@JFreegman JFreegman deleted the group_array_entry_cleanup branch January 11, 2024 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Internal code cleanup, possibly affecting semantics, e.g. deleting a deprecated feature.
Development

Successfully merging this pull request may close these issues.

4 participants