ronantakizawa/codereview-bench huggingface dataset:
Repository: valkey-io/valkey
File: src/connection.c (C)

Code before:
    /* Unlike the TCP and Unix connections, the TLS one can be missing
     * So we need the cached pointer to handle NULL correctly too. */
    if (!cached) {
        cached = 1;
        ct_tls = connectionByType(CONN_TYPE_TLS);
    }

    return ct_tls;
}

/* Cache Unix connection type, query it by string once */
ConnectionType *connectionTypeUnix(void) {
    static ConnectionType *ct_unix = NULL;

    if (ct_unix != NULL) return ct_unix;

    ct_unix = connectionByType(CONN_TYPE_UNIX);
    return ct_unix;
}

void connTypeCleanupAll(void) {
    ConnectionType *ct;
    int type;

    // type = 0 is an invalid conn type.
    for (type = 1; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (!ct) continue;

        if (ct->cleanup) ct->cleanup();
    }
}

/* walk all the connection types until has pending data */
int connTypeHasPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    // type = 0 is an invalid conn type.
    for (type = 1; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->has_pending_data && (ret = ct->has_pending_data())) {
            return ret;
        }
    }

    return ret;
}

/* walk all the connection types and process pending data for each connection type */

Review comment (suggestion):
```suggestion
    for (type = 0; type < CONN_TYPE_MAX; type++) {
```

Code after:
     * So we need the cached pointer to handle NULL correctly too. */
    if (!cached) {
        cached = 1;
        ct_tls = connectionByType(CONN_TYPE_TLS);
    }

    return ct_tls;
}

/* Cache Unix connection type, query it by string once */
ConnectionType *connectionTypeUnix(void) {
    static ConnectionType *ct_unix = NULL;

    if (ct_unix != NULL) return ct_unix;

    ct_unix = connectionByType(CONN_TYPE_UNIX);
    return ct_unix;
}

void connTypeCleanupAll(void) {
    ConnectionType *ct;
    int type;

    for (type = 0; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (!ct) continue;

        if (ct->cleanup) ct->cleanup();
    }
}

/* walk all the connection types until has pending data */
int connTypeHasPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    for (type = 0; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->has_pending_data && (ret = ct->has_pending_data())) {
            return ret;
        }
    }

    return ret;
}

/* walk all the connection types and process pending data for each connection type */
int connTypeProcessPendingData(void) {
    ConnectionType *ct;
    int type;
Repository: valkey-io/valkey
File: src/connection.c (C)

Code before:

    ct_unix = connectionByType(CONN_TYPE_UNIX);
    return ct_unix;
}

void connTypeCleanupAll(void) {
    ConnectionType *ct;
    int type;

    // type = 0 is an invalid conn type.
    for (type = 1; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (!ct) continue;

        if (ct->cleanup) ct->cleanup();
    }
}

/* walk all the connection types until has pending data */
int connTypeHasPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    // type = 0 is an invalid conn type.
    for (type = 1; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->has_pending_data && (ret = ct->has_pending_data())) {
            return ret;
        }
    }

    return ret;
}

/* walk all the connection types and process pending data for each connection type */
int connTypeProcessPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    // type = 0 is an invalid conn type.
    for (type = 1; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->process_pending_data) {
            ret += ct->process_pending_data();
        }
    }

    return ret;
}

Review comment (suggestion):
```suggestion
    for (type = 0; type < CONN_TYPE_MAX; type++) {
```

Code after:
    ct_unix = connectionByType(CONN_TYPE_UNIX);
    return ct_unix;
}

void connTypeCleanupAll(void) {
    ConnectionType *ct;
    int type;

    for (type = 0; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (!ct) continue;

        if (ct->cleanup) ct->cleanup();
    }
}

/* walk all the connection types until has pending data */
int connTypeHasPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    for (type = 0; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->has_pending_data && (ret = ct->has_pending_data())) {
            return ret;
        }
    }

    return ret;
}

/* walk all the connection types and process pending data for each connection type */
int connTypeProcessPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    for (type = 0; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->process_pending_data) {
            ret += ct->process_pending_data();
        }
    }

    return ret;
}

sds getListensInfoString(sds info) {
    for (int j = 0; j < CONN_TYPE_MAX; j++) {
        connListener *listener = &server.listeners[j];
Repository: valkey-io/valkey
File: src/connection.c (C)

Code before:

/* walk all the connection types until has pending data */
int connTypeHasPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    // type = 0 is an invalid conn type.
    for (type = 1; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->has_pending_data && (ret = ct->has_pending_data())) {
            return ret;
        }
    }

    return ret;
}

/* walk all the connection types and process pending data for each connection type */
int connTypeProcessPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    // type = 0 is an invalid conn type.
    for (type = 1; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->process_pending_data) {
            ret += ct->process_pending_data();
        }
    }

    return ret;
}

sds getListensInfoString(sds info) {
    // server.listeners[0] is reserved.
    for (int j = 1; j < CONN_TYPE_MAX; j++) {
        connListener *listener = &server.listeners[j];
        if (listener->ct == NULL) continue;

        info = sdscatfmt(info, "listener%i:name=%s", j, getConnectionTypeName(listener->ct->get_type()));
        for (int i = 0; i < listener->count; i++) {
            info = sdscatfmt(info, ",bind=%s", listener->bindaddr[i]);
        }

        if (listener->port) info = sdscatfmt(info, ",port=%i", listener->port);

        info = sdscatfmt(info, "\r\n");
    }


Review comment (suggestion):
```suggestion
    for (type = 0; type < CONN_TYPE_MAX; type++) {
```

Code after:
int connTypeHasPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    for (type = 0; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->has_pending_data && (ret = ct->has_pending_data())) {
            return ret;
        }
    }

    return ret;
}

/* walk all the connection types and process pending data for each connection type */
int connTypeProcessPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    for (type = 0; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->process_pending_data) {
            ret += ct->process_pending_data();
        }
    }

    return ret;
}

sds getListensInfoString(sds info) {
    for (int j = 0; j < CONN_TYPE_MAX; j++) {
        connListener *listener = &server.listeners[j];
        if (listener->ct == NULL) continue;

        info = sdscatfmt(info, "listener%i:name=%s", j, getConnectionTypeName(listener->ct->get_type()));
        for (int i = 0; i < listener->count; i++) {
            info = sdscatfmt(info, ",bind=%s", listener->bindaddr[i]);
        }

        if (listener->port) info = sdscatfmt(info, ",port=%i", listener->port);

        info = sdscatfmt(info, "\r\n");
    }

    return info;
}
Repository: valkey-io/valkey
File: src/connection.c (C)

Code before:
        }
    }

    return ret;
}

/* walk all the connection types and process pending data for each connection type */
int connTypeProcessPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    // type = 0 is an invalid conn type.
    for (type = 1; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->process_pending_data) {
            ret += ct->process_pending_data();
        }
    }

    return ret;
}

sds getListensInfoString(sds info) {
    // server.listeners[0] is reserved.
    for (int j = 1; j < CONN_TYPE_MAX; j++) {
        connListener *listener = &server.listeners[j];
        if (listener->ct == NULL) continue;

        info = sdscatfmt(info, "listener%i:name=%s", j, getConnectionTypeName(listener->ct->get_type()));
        for (int i = 0; i < listener->count; i++) {
            info = sdscatfmt(info, ",bind=%s", listener->bindaddr[i]);
        }

        if (listener->port) info = sdscatfmt(info, ",port=%i", listener->port);

        info = sdscatfmt(info, "\r\n");
    }

    return info;
}


Review comment (suggestion):
```suggestion
    for (int j = 0; j < CONN_TYPE_MAX; j++) {
```

Code after:
    return ret;
}

/* walk all the connection types and process pending data for each connection type */
int connTypeProcessPendingData(void) {
    ConnectionType *ct;
    int type;
    int ret = 0;

    for (type = 0; type < CONN_TYPE_MAX; type++) {
        ct = connTypes[type];
        if (ct && ct->process_pending_data) {
            ret += ct->process_pending_data();
        }
    }

    return ret;
}

sds getListensInfoString(sds info) {
    for (int j = 0; j < CONN_TYPE_MAX; j++) {
        connListener *listener = &server.listeners[j];
        if (listener->ct == NULL) continue;

        info = sdscatfmt(info, "listener%i:name=%s", j, getConnectionTypeName(listener->ct->get_type()));
        for (int i = 0; i < listener->count; i++) {
            info = sdscatfmt(info, ",bind=%s", listener->bindaddr[i]);
        }

        if (listener->port) info = sdscatfmt(info, ",port=%i", listener->port);

        info = sdscatfmt(info, "\r\n");
    }

    return info;
}
Repository: valkey-io/valkey
File: src/io_threads.h (C/C++)

Code before:
#ifndef IO_THREADS_H
#define IO_THREADS_H

#include "server.h"

#define TIMESPEC_DIFF_US(start_time, end_time)                                     \
    (((long long)(end_time).tv_sec - (long long)(start_time).tv_sec) * 1000000LL + \
     ((long long)(end_time).tv_nsec - (long long)(start_time).tv_nsec) / 1000L)

void initIOThreads(void);
void killIOThreads(void);
int inMainThread(void);
int trySendReadToIOThreads(client *c);
int trySendWriteToIOThreads(client *c);
int tryOffloadFreeObjToIOThreads(robj *o);
int tryOffloadFreeArgvToIOThreads(client *c, int argc, robj **argv);
void adjustIOThreadsByEventLoad(int numevents, int increase_only);
void drainIOThreadsQueue(void);
void trySendPollJobToIOThreads(void);
int trySendAcceptToIOThreads(connection *conn);
int updateIOThreads(const char **err);
long long getIOThreadUsefulTimeMicroseconds(int id);
long long getIOThreadUptimeMicroseconds(int id);

#endif /* IO_THREADS_H */


Review comment (suggestion):
`static inline` function is almost always better than macro. We get type checking and better readability. It's still defined in the header file like a macro.

```suggestion
static inline long long timespec_diff_us(struct timespec start_time, struct timespec end_time) {
    return ((long long)end_time.tv_sec - (long long)start_time.tv_sec) * 1000000LL +
           ((long long)end_time.tv_nsec - (long long)start_time.tv_nsec) / 1000LL;
}
```

Code after:
#ifndef IO_THREADS_H
#define IO_THREADS_H

#include "server.h"

void initIOThreads(void);
void killIOThreads(void);
int inMainThread(void);
int trySendReadToIOThreads(client *c);
int trySendWriteToIOThreads(client *c);
int tryOffloadFreeObjToIOThreads(robj *o);
int tryOffloadFreeArgvToIOThreads(client *c, int argc, robj **argv);
void adjustIOThreadsByEventLoad(int numevents, int increase_only);
void drainIOThreadsQueue(void);
void trySendPollJobToIOThreads(void);
int trySendAcceptToIOThreads(connection *conn);
int updateIOThreads(const char **err);
long long getIOThreadActiveTimeMicroseconds(int id);

#endif /* IO_THREADS_H */
Repository: valkey-io/valkey
File: src/version.h (C/C++)

Code before:
/* These macros are used where the server name is printed in logs and replies.
 * Note the difference in the first letter "V" vs "v". SERVER_TITLE is used in
 * readable text like log messages and SERVER_NAME is used in INFO fields and
 * similar. */
#define SERVER_NAME "valkey"
#define SERVER_TITLE "Valkey"
#define VALKEY_VERSION "8.9.240"
#define VALKEY_VERSION_NUM 0x000809f0
/* The release stage is used in order to provide release status information.
 * In unstable branch the status is always "dev".
 * During release process the status will be set to rc1,rc2...rcN.
 * When the version is released the status will be "ga". */
#define VALKEY_RELEASE_STAGE "rc1"

/* Redis OSS compatibility version, should never
 * exceed 7.2.x. */
#define REDIS_VERSION "7.2.4"
#define REDIS_VERSION_NUM 0x00070204


Review comment (suggestion):
```suggestion
#define VALKEY_VERSION "9.0.0"
#define VALKEY_VERSION_NUM 0x00090000
```

Code after:
/* These macros are used where the server name is printed in logs and replies.
 * Note the difference in the first letter "V" vs "v". SERVER_TITLE is used in
 * readable text like log messages and SERVER_NAME is used in INFO fields and
 * similar. */
#define SERVER_NAME "valkey"
#define SERVER_TITLE "Valkey"
#define VALKEY_VERSION "9.0.0"
#define VALKEY_VERSION_NUM 0x00090000
/* The release stage is used in order to provide release status information.
 * In unstable branch the status is always "dev".
 * During release process the status will be set to rc1,rc2...rcN.
 * When the version is released the status will be "ga". */
#define VALKEY_RELEASE_STAGE "rc1"

/* Redis OSS compatibility version, should never
 * exceed 7.2.x. */
#define REDIS_VERSION "7.2.4"
#define REDIS_VERSION_NUM 0x00070204
Repository: valkey-io/valkey
File: src/valkey-benchmark.c (C)

Code before:
    "__rand_5th__", "__rand_6th__", "__rand_7th__", "__rand_8th__", "__rand_9th__"};

struct benchmarkThread;
struct clusterNode;
struct serverConfig;

/* Read from replica options */
typedef enum readFromReplica {
    FROM_PRIMARY_ONLY = 0, /* default option */
    FROM_REPLICA_ONLY,
    FROM_ALL
} readFromReplica;

static struct config {
    aeEventLoop *el;
    enum valkeyConnectionType ct;
    cliConnInfo conn_info;
    int tls;
    int mptcp;
    struct cliSSLconfig sslconfig;
    int numclients;
    _Atomic int liveclients;
    int requests;
    int duration;
    _Atomic int warmup_duration;
    _Atomic int current_warmup_duration;
    _Atomic int requests_issued;
    _Atomic int requests_finished;
    _Atomic int previous_requests_finished;
    int last_printed_bytes;
    long long previous_tick;
    int keysize;
    int datasize;
    int replace_placeholders;
    int keyspacelen;
    int sequential_replacement;
    int keepalive;
    int pipeline;
    long long start;
    long long totlatency;
    const char *title;
    list *clients;
    list *paused_clients;
    int quiet;
    int csv;
    int loop;
    int idlemode;
    sds input_dbnumstr;
    char *tests;
    int stdinarg; /* get last arg from stdin. (-x option) */
    int precision;

Review comment (suggestion):
`warmup_duration` is only set when parsing command line arguments and is not modified after that, so it doesn't seem to need to be atomic.

```suggestion
    int warmup_duration;
    _Atomic int current_warmup_duration;
```

Code after:
    "__rand_5th__", "__rand_6th__", "__rand_7th__", "__rand_8th__", "__rand_9th__"};

struct benchmarkThread;
struct clusterNode;
struct serverConfig;

/* Read from replica options */
typedef enum readFromReplica {
    FROM_PRIMARY_ONLY = 0, /* default option */
    FROM_REPLICA_ONLY,
    FROM_ALL
} readFromReplica;

static struct config {
    aeEventLoop *el;
    enum valkeyConnectionType ct;
    cliConnInfo conn_info;
    int tls;
    int mptcp;
    struct cliSSLconfig sslconfig;
    int numclients;
    _Atomic int liveclients;
    int requests;
    int duration;
    int warmup_duration;
    _Atomic int current_warmup_duration;
    _Atomic int requests_issued;
    _Atomic int requests_finished;
    _Atomic int previous_requests_finished;
    int last_printed_bytes;
    long long previous_tick;
    int keysize;
    int datasize;
    int replace_placeholders;
    int keyspacelen;
    int sequential_replacement;
    int keepalive;
    int pipeline;
    long long start;
    long long totlatency;
    const char *title;
    list *clients;
    list *paused_clients;
    int quiet;
    int csv;
    int loop;
    int idlemode;
    sds input_dbnumstr;
    char *tests;
    int stdinarg; /* get last arg from stdin. (-x option) */
    int precision;
Repository: valkey-io/valkey
File: src/entry.c (C)

Code before:
#include <stdbool.h>
#include "server.h"
#include "serverassert.h"
#include "entry.h"

#include <stdbool.h>

/*-----------------------------------------------------------------------------
 * Entry API
 *----------------------------------------------------------------------------*/

/* The entry pointer is the field sds. We encode the entry layout type
 * in the field SDS header. Field type SDS_TYPE_5 doesn't have any spare bits to
 * encode this so we use it only for the first layout type.
 *
 * Entry with embedded value, used for small sizes. The value is stored as
 * SDS_TYPE_8. The field can use any SDS type.
 *
 * Entry can also have expiration timestamp, which is the UNIX timestamp for it to be expired.
 * For aligned fast access, we keep the expiry timestamp prior to the start of the sds header.
 *
 *     +--------------+--------------+---------------+
 *     | Expiration   | field        | value         |
 *     | 1234567890LL | hdr "foo" \0 | hdr8 "bar" \0 |
 *     +--------------+--------------+---------------+
 *
 * Entry with value pointer, used for larger fields and values. The field is SDS
 * type 8 or higher.
 *
 *     +--------------+-------+--------------+
 *     | Expiration   | value | field        |
 *     | 1234567890LL | ptr   | hdr "foo" \0 |
 *     +--------------+---^---+--------------+
 *                        |

Review comment (suggestion):
there are 4 right? expiration existence only multiply the past 2 encoding types (embedded value and non-embedded value)

Code after:
#include "server.h"
#include "serverassert.h"
#include "entry.h"

/*-----------------------------------------------------------------------------
 * Entry Implementation
 *----------------------------------------------------------------------------*/

/* There are 3 different formats for the "entry".  In all cases, the "entry" pointer points into the
 * allocation and is identical to the "field" sds pointer.
 *
 * Type 1: Field sds type is an SDS_TYPE_5
 *     With this type, both the key and value are embedded in the entry.  Expiration is not allowed
 *     as the SDS_TYPE_5 (on field) doesn't contain any aux bits to encode the existence of an
 *     expiration.  Extra padding is included in the value to the size of the physical block.
 *
 *             entry
 *               |
 *     +---------V------------+----------------------------+
 *     |       Field          |      Value                 |
 *     | sdshdr5 | "foo" \0   | sdshdr8 "bar" \0 (padding) |
 *     +---------+------------+----------------------------+
 *
 *     Identified by: field sds type is SDS_TYPE_5
 *
 *
 * Type 2: Field sds type is an SDS_TYPE_8 type
 *     With this type, both the key and value are embedded.  Extra bits in the sdshdr8 (on field)
 *     are used to encode aux flags which may indicate the presence of an optional expiration.
 *     Extra padding is included in the value to the size of the physical block.
 *
 *                            entry
 *                              |
 *     +--------------+---------V------------+----------------------------+
Repository: valkey-io/valkey
File: tests/modules/cluster.c (C)

Code before:
#include "valkeymodule.h"
#include <string.h>

#define UNUSED(x) (void)(x)

void cluster_timer_handler(ValkeyModuleCtx *ctx, void *data) {
    VALKEYMODULE_NOT_USED(data);

    ValkeyModuleCallReply *rep = ValkeyModule_Call(ctx, "CLUSTER", "c", "SLOTS");

    if (rep) {
        ValkeyModule_Log(ctx, "notice", "Timer: CLUSTER SLOTS success");
        ValkeyModule_FreeCallReply(rep);
    } else {
        ValkeyModule_Log(ctx, "warning", "Timer: CLUSTER SLOTS failed");
    }
}

int test_start_cluster_timer(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
    VALKEYMODULE_NOT_USED(argv);
    VALKEYMODULE_NOT_USED(argc);

    ValkeyModule_CreateTimer(ctx, 1, cluster_timer_handler, NULL);

    return ValkeyModule_ReplyWithSimpleString(ctx, "OK");
}


int test_cluster_slots(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
    UNUSED(argv);

    if (argc != 1) return ValkeyModule_WrongArity(ctx);

    ValkeyModuleCallReply *rep = ValkeyModule_Call(ctx, "CLUSTER", "c", "SLOTS");
    if (!rep) {
        ValkeyModule_ReplyWithError(ctx, "ERR NULL reply returned");
    } else {

Review comment (suggestion):
We could peek at what we got (as Allen suggested earlier) to check that we got an array. We could also get an error reply (VALKEYMODULE_REPLY_ERROR) for example if cluster mode is not enabled or in some other cases.

```suggestion
        if (ValkeyModule_CallReplyType(rep) == VALKEYMODULE_REPLY_ARRAY) {
            ValkeyModule_Log(ctx, "notice", "Timer: CLUSTER SLOTS success");
        } else {
            ValkeyModule_Log(ctx, "notice",
                             "Timer: CLUSTER SLOTS unexpected reply type %d",
                             ValkeyModule_CallReplyType(rep));
        }
```

Code after:
#include "valkeymodule.h"
#include <string.h>

#define UNUSED(x) (void)(x)

void cluster_timer_handler(ValkeyModuleCtx *ctx, void *data) {
    VALKEYMODULE_NOT_USED(data);

    ValkeyModuleCallReply *rep = ValkeyModule_Call(ctx, "CLUSTER", "c", "SLOTS");

    if (rep) {
        if (ValkeyModule_CallReplyType(rep) == VALKEYMODULE_REPLY_ARRAY) {
            ValkeyModule_Log(ctx, "notice", "Timer: CLUSTER SLOTS success");
        } else {
            ValkeyModule_Log(ctx, "notice",
                             "Timer: CLUSTER SLOTS unexpected reply type %d",
                             ValkeyModule_CallReplyType(rep));
        }
        ValkeyModule_FreeCallReply(rep);
    } else {
        ValkeyModule_Log(ctx, "warning", "Timer: CLUSTER SLOTS failed");
    }
}

int test_start_cluster_timer(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
    VALKEYMODULE_NOT_USED(argv);
    VALKEYMODULE_NOT_USED(argc);

    ValkeyModule_CreateTimer(ctx, 1, cluster_timer_handler, NULL);

    return ValkeyModule_ReplyWithSimpleString(ctx, "OK");
}


int test_cluster_slots(ValkeyModuleCtx *ctx, ValkeyModuleString **argv, int argc) {
    UNUSED(argv);