search

TokTok / c-toxcore

The future of online communications.
https://tox.chat
GNU General Public License v3.0
2.21k stars 280 forks source link

cleanup: correct a few nullable annotations #2685

Closed JFreegman closed 4 months ago

JFreegman commented 4 months ago

This change is Reviewable

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (623e3ee) 73.11% compared to head (fbe3c19) 73.05%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2685 +/- ## ========================================== - Coverage 73.11% 73.05% -0.06% ========================================== Files 148 148 Lines 30486 30486 ========================================== - Hits 22291 22273 -18 - Misses 8195 8213 +18 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

JFreegman commented 4 months ago

Does the length being non-zero guarantee that the data is not NULL? In that case these changes would make sense.

But if the length is user-provided, then no.

Reviewable status: 1 change requests, 0 of 1 approvals obtained

There should never be an instance where a non-zero length is paired with a null data pointer. That would indicate a serious bug. The length is never user-provided. These notations are for telling static analyzers what to expect, and if they see something unexpected, they'll tell us about it. Since both these functions are able to handle both null and non-null data pointers, analyzers shouldn't complain if they see either case.

If there were a bug and we passed a null pointer to net_unpack() for example, then the analyzer would warn us for that specific case.

nurupo commented 4 months ago

These notations are for telling static analyzers what to expect, and if they see something unexpected, they'll tell us about it. Since both these functions are able to handle both null and non-null data pointers, analyzers shouldn't complain if they see either case.

Yep. And there are actually a lot more of these attributes we could use, for example marking which arguments are read-only and which are read-write, or which argument is expecting a null-terminated C-string https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

nurupo commented 4 months ago

Too bad we can't use those in the API as they are not really portable.