cleanup: Upgrade to clang-tidy-17 and fix some warnings.

[iphydf]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (b7f9367) 69.00% compared to head (ef4897a) 69.02%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2503      +/-   ##
==========================================
+ Coverage   69.00%   69.02%   +0.01%     
==========================================
  Files          89       89              
  Lines       27768    27769       +1     
==========================================
+ Hits        19162    19167       +5     
+ Misses       8606     8602       -4     

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

[nurupo]

Reviewed 17 of 17 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @iphydf)

---

other/analysis/run-clang-tidy line 110 at r1 (raw file):

# TODO(iphydf): These two trip on list.c. Investigate why.
CHECKS="$CHECKS,-clang-analyzer-core.NonNullParamChecker"
CHECKS="$CHECKS,-clang-analyzer-unix.Malloc"

Want to note that, while not clang-tidy, clang-analyze still trips on these:

./toxcore/list.c:233:9: warning: Array access (via field 'ids') results in a null pointer dereference [core.NullDereference]
    if (list->ids[i] != id) {
        ^~~~~~~~~~~~
./toxcore/list.c:249:5: warning: Use of memory after it is freed [unix.Malloc]
    memmove(list->data + i * list->element_size, list->data + (i + 1) * list->element_size,
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Which is interesting, you'd think clang-analyzer-core.* are the same checks as what clang --analyze uses, but clearly clang-tidy no longer trips on these but clang --analyze still does.

Anyway, just something I wanted to point out, I don't think this is actionable or anything, i.e. I'm not requesting any changes here.

---

other/bootstrap_daemon/src/tox-bootstrapd.c line 228 at r1 (raw file):

        case CLI_STATUS_ERROR:
            return 1;
    }

1. Out of curiosity, what was the warning message for using exit() / rationale for avoiding to use it that the tool printed?

2. I do see a problem with a function calling exit() internally, as the caller might not expect the function to do so, but rather than lifting the exit() call into the main() function as an early return by having functions return the enum, I wonder if renaming handle_command_line_arguments() to handle_command_line_arguments_or_die() and similarly daemonize() to daemonize_or_die() would be clearer and cleaner -- without needing the enum and switches. Wouldn't fix a warning if it's looking for exit() calls though, so if this is done to fix a warning then fair enough, let's keep this. (Though making code more clunky just to please static analysis tools is a bit annoying, shouldn't the tools work for us and not the other way around?).

[nurupo]

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

[nurupo]

other/analysis/run-clang-tidy line 110 at r1 (raw file):

Previously, nurupo wrote…

Want to note that, while not clang-tidy, clang-analyze still trips on these:

./toxcore/list.c:233:9: warning: Array access (via field 'ids') results in a null pointer dereference [core.NullDereference]
    if (list->ids[i] != id) {
        ^~~~~~~~~~~~
./toxcore/list.c:249:5: warning: Use of memory after it is freed [unix.Malloc]
    memmove(list->data + i * list->element_size, list->data + (i + 1) * list->element_size,
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Which is interesting, you'd think clang-analyzer-core.* are the same checks as what clang --analyze uses, but clearly clang-tidy no longer trips on these but clang --analyze still does.

Anyway, just something I wanted to point out, I don't think this is actionable or anything, i.e. I'm not requesting any changes here.

Could also be the difference between the versions of clang used, Alpine vs Ubuntu (clang-analyze job is defined in .circleci/config.yml).

[iphydf]

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

---

other/analysis/run-clang-tidy line 110 at r1 (raw file):

Previously, nurupo wrote…

Could also be the difference between the versions of clang used, Alpine vs Ubuntu (clang-analyze job is defined in .circleci/config.yml).

Might also be different compile options. I can look into that later.

---

other/bootstrap_daemon/src/tox-bootstrapd.c line 228 at r1 (raw file):

Previously, nurupo wrote…

1. Out of curiosity, what was the warning message for using exit() / rationale for avoiding to use it that the tool printed?

2. I do see a problem with a function calling exit() internally, as the caller might not expect the function to do so, but rather than lifting the exit() call into the main() function as an early return by having functions return the enum, I wonder if renaming handle_command_line_arguments() to handle_command_line_arguments_or_die() and similarly daemonize() to daemonize_or_die() would be clearer and cleaner -- without needing the enum and switches. Wouldn't fix a warning if it's looking for exit() calls though, so if this is done to fix a warning then fair enough, let's keep this. (Though making code more clunky just to please static analysis tools is a bit annoying, shouldn't the tools work for us and not the other way around?).

clang-tidy doesn't like exit() because it's not thread-safe. There's no way to disable only exit() from that list of thread-unsafe functions. It's nice to have that analysis around the rest of the code.

[nurupo]

Reviewable status: complete! 1 of 1 approvals obtained