From 0cef46ee9190ff9eac52dc52ecb1743a27ea70be Mon Sep 17 00:00:00 2001 From: iphydf Date: Thu, 31 Aug 2023 14:21:36 +0000 Subject: [PATCH] cleanup: Fix a few more clang-tidy warnings. --- .clang-tidy | 2 ++ other/analysis/run-clang-tidy | 31 ++++++++++++------- .../docker/tox-bootstrapd.sha256 | 2 +- other/bootstrap_daemon/src/tox-bootstrapd.c | 4 --- toxcore/DHT.c | 4 +-- toxcore/mono_time.c | 2 +- toxcore/net_crypto.c | 5 ++- toxcore/tox.c | 17 ++++++---- 8 files changed, 37 insertions(+), 30 deletions(-) diff --git a/.clang-tidy b/.clang-tidy index 2a396a2c..5ad92a14 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -36,3 +36,5 @@ CheckOptions: - key: llvmlibc-restrict-system-libc-headers.Includes value: "arpa/inet.h,assert.h,ctype.h,errno.h,fcntl.h,getopt.h,libconfig.h,linux/netdevice.h,math.h,netdb.h,netinet/in.h,opus.h,pthread.h,signal.h,sodium/crypto_scalarmult_curve25519.h,sodium.h,sodium/randombytes.h,stdio.h,stdlib.h,string.h,sys/ioctl.h,syslog.h,sys/resource.h,sys/socket.h,sys/stat.h,sys/time.h,sys/types.h,time.h,unistd.h,vpx/vp8cx.h,vpx/vp8dx.h,vpx/vpx_decoder.h,vpx/vpx_encoder.h,vpx/vpx_image.h" + - key: hicpp-signed-bitwise.IgnorePositiveIntegerLiterals + value: true diff --git a/other/analysis/run-clang-tidy b/other/analysis/run-clang-tidy index ac9175e8..a87b517b 100755 --- a/other/analysis/run-clang-tidy +++ b/other/analysis/run-clang-tidy @@ -14,7 +14,9 @@ CHECKS="$CHECKS,-cppcoreguidelines-init-variables" # readability issue. CHECKS="$CHECKS,-readability-identifier-length" -# This finds all the do {} while (0) loops and tells us to unroll them. +# Altera checks are for GPUs (OpenCL). Our code doesn't run on GPUs. +CHECKS="$CHECKS,-altera-id-dependent-backward-branch" +CHECKS="$CHECKS,-altera-struct-pack-align" CHECKS="$CHECKS,-altera-unroll-loops" # We target systems other than Android, so we don't want to use non-standard @@ -27,6 +29,10 @@ CHECKS="$CHECKS,-bugprone-reserved-identifier" CHECKS="$CHECKS,-cert-dcl37-c" CHECKS="$CHECKS,-cert-dcl51-cpp" +# Too restrictive. This makes sense if the branch clone is very large, but not +# if it's a single line. It can make the code less readable. +CHECKS="$CHECKS,-bugprone-branch-clone" + # We intentionally send some not null-terminated strings in tests and use it for # the toxencryptsave magic number. CHECKS="$CHECKS,-bugprone-not-null-terminated-result" @@ -43,6 +49,14 @@ CHECKS="$CHECKS,-readability-else-after-return" # functions otherwise. CHECKS="$CHECKS,-readability-redundant-control-flow" +# These are incredibly annoying, because things like +# uint16_t a = 0, b = 1, c = a > b ? a : b; +# ^ +# Trip the checker, which is true, because of integer promotion, but also not +# very helpful as a diagnostic. +CHECKS="$CHECKS,-bugprone-narrowing-conversions" +CHECKS="$CHECKS,-cppcoreguidelines-narrowing-conversions" + # TODO(iphydf): We might want some of these. For the ones we don't want, add a # comment explaining why not. CHECKS="$CHECKS,-clang-analyzer-optin.performance.Padding" @@ -51,21 +65,13 @@ CHECKS="$CHECKS,-misc-unused-parameters" CHECKS="$CHECKS,-readability-function-cognitive-complexity" # TODO(iphydf): Maybe fix these? -CHECKS="$CHECKS,-altera-id-dependent-backward-branch" -CHECKS="$CHECKS,-altera-struct-pack-align" -CHECKS="$CHECKS,-bugprone-branch-clone" CHECKS="$CHECKS,-bugprone-easily-swappable-parameters" CHECKS="$CHECKS,-bugprone-implicit-widening-of-multiplication-result" CHECKS="$CHECKS,-bugprone-integer-division" -CHECKS="$CHECKS,-bugprone-narrowing-conversions" -CHECKS="$CHECKS,-clang-analyzer-core.NonNullParamChecker" CHECKS="$CHECKS,-clang-analyzer-core.NullDereference" -CHECKS="$CHECKS,-clang-analyzer-optin.portability.UnixAPI" -CHECKS="$CHECKS,-clang-analyzer-unix.Malloc" CHECKS="$CHECKS,-clang-analyzer-valist.Uninitialized" CHECKS="$CHECKS,-concurrency-mt-unsafe" CHECKS="$CHECKS,-cppcoreguidelines-avoid-non-const-global-variables" -CHECKS="$CHECKS,-cppcoreguidelines-narrowing-conversions" CHECKS="$CHECKS,-misc-no-recursion" # TODO(iphydf): Probably fix these. @@ -75,15 +81,16 @@ CHECKS="$CHECKS,-google-readability-casting" CHECKS="$CHECKS,-modernize-macro-to-enum" CHECKS="$CHECKS,-readability-magic-numbers" +# TODO(iphydf): These two trip on list.c. Investigate why. +CHECKS="$CHECKS,-clang-analyzer-core.NonNullParamChecker" +CHECKS="$CHECKS,-clang-analyzer-unix.Malloc" + ERRORS="*" # TODO(iphydf): Fix these. ERRORS="$ERRORS,-bugprone-macro-parentheses" -ERRORS="$ERRORS,-bugprone-posix-return" -ERRORS="$ERRORS,-bugprone-signed-char-misuse" ERRORS="$ERRORS,-cert-err34-c" ERRORS="$ERRORS,-cert-str34-c" -ERRORS="$ERRORS,-hicpp-uppercase-literal-suffix" ERRORS="$ERRORS,-readability-suspicious-call-argument" set -eux diff --git a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 index dad4290e..61ec91a6 100644 --- a/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 +++ b/other/bootstrap_daemon/docker/tox-bootstrapd.sha256 @@ -1 +1 @@ -3089925fd022dec8ec335435caf56f6ad5ccd2bf916f92f86a544da0ff654501 /usr/local/bin/tox-bootstrapd +619d28e6ecd0dbfbf8727753d44aa7eb1d8baa53cfcd4067f426141b4c132784 /usr/local/bin/tox-bootstrapd diff --git a/other/bootstrap_daemon/src/tox-bootstrapd.c b/other/bootstrap_daemon/src/tox-bootstrapd.c index ada2334b..8d0e3183 100644 --- a/other/bootstrap_daemon/src/tox-bootstrapd.c +++ b/other/bootstrap_daemon/src/tox-bootstrapd.c @@ -178,11 +178,7 @@ static LOG_LEVEL logger_level_to_log_level(Logger_Level level) { switch (level) { case LOGGER_LEVEL_TRACE: - return LOG_LEVEL_INFO; - case LOGGER_LEVEL_DEBUG: - return LOG_LEVEL_INFO; - case LOGGER_LEVEL_INFO: return LOG_LEVEL_INFO; diff --git a/toxcore/DHT.c b/toxcore/DHT.c index ca96dbe3..75907645 100644 --- a/toxcore/DHT.c +++ b/toxcore/DHT.c @@ -2466,9 +2466,7 @@ static uint16_t list_nodes(const Random *rng, const Client_data *list, size_t le } if (!assoc_timeout(cur_time, &list[i - 1].assoc6)) { - if (assoc == nullptr) { - assoc = &list[i - 1].assoc6; - } else if ((random_u08(rng) % 2) != 0) { + if (assoc == nullptr || (random_u08(rng) % 2) != 0) { assoc = &list[i - 1].assoc6; } } diff --git a/toxcore/mono_time.c b/toxcore/mono_time.c index e03046f7..418deec4 100644 --- a/toxcore/mono_time.c +++ b/toxcore/mono_time.c @@ -138,7 +138,7 @@ Mono_Time *mono_time_new(const Memory *mem, mono_time_current_time_cb *current_t return nullptr; } - if (pthread_rwlock_init(mono_time->time_update_lock, nullptr) < 0) { + if (pthread_rwlock_init(mono_time->time_update_lock, nullptr) != 0) { mem_delete(mem, mono_time->time_update_lock); mem_delete(mem, mono_time); return nullptr; diff --git a/toxcore/net_crypto.c b/toxcore/net_crypto.c index 7fb4b3a8..4d2aaac1 100644 --- a/toxcore/net_crypto.c +++ b/toxcore/net_crypto.c @@ -3066,9 +3066,8 @@ bool crypto_connection_status(const Net_Crypto *c, int crypt_connection_id, bool const uint64_t current_time = mono_time_get(c->mono_time); - if ((UDP_DIRECT_TIMEOUT + conn->direct_lastrecv_timev4) > current_time) { - *direct_connected = true; - } else if ((UDP_DIRECT_TIMEOUT + conn->direct_lastrecv_timev6) > current_time) { + if ((UDP_DIRECT_TIMEOUT + conn->direct_lastrecv_timev4) > current_time || + (UDP_DIRECT_TIMEOUT + conn->direct_lastrecv_timev6) > current_time) { *direct_connected = true; } } diff --git a/toxcore/tox.c b/toxcore/tox.c index d17e61b7..4053b873 100644 --- a/toxcore/tox.c +++ b/toxcore/tox.c @@ -812,12 +812,17 @@ Tox *tox_new(const struct Tox_Options *options, Tox_Err_New *error) tox->m = new_messenger(tox->mono_time, tox->sys.mem, tox->sys.rng, tox->sys.ns, &m_options, &m_error); if (tox->m == nullptr) { - if (m_error == MESSENGER_ERROR_PORT) { - SET_ERROR_PARAMETER(error, TOX_ERR_NEW_PORT_ALLOC); - } else if (m_error == MESSENGER_ERROR_TCP_SERVER) { - SET_ERROR_PARAMETER(error, TOX_ERR_NEW_PORT_ALLOC); - } else { - SET_ERROR_PARAMETER(error, TOX_ERR_NEW_MALLOC); + switch (m_error) { + case MESSENGER_ERROR_PORT: + case MESSENGER_ERROR_TCP_SERVER: { + SET_ERROR_PARAMETER(error, TOX_ERR_NEW_PORT_ALLOC); + break; + } + case MESSENGER_ERROR_OTHER: + case MESSENGER_ERROR_NONE: { + SET_ERROR_PARAMETER(error, TOX_ERR_NEW_MALLOC); + break; + } } mono_time_free(tox->sys.mem, tox->mono_time);