ronantakizawa/codereview-bench huggingface dataset:
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 "255.255.255"
#define VALKEY_VERSION_NUM 0x00ffffff
/* 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 "dev"

/* 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
 * During release process the status will be set to rc1,rc2...rcN.
```

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 "255.255.255"
#define VALKEY_VERSION_NUM 0x00ffffff
/* 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 "dev"

/* 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/unit/test_bitops.c (C)

Code before:
#include "../zmalloc.h"

extern long long popcountScalar(void *s, long count);
#ifdef HAVE_AVX2
extern long long popcountAVX2(void *s, long count);
#endif

static long long bitcount(void *s, long count) {
    long long bits = 0;
    uint8_t *p = (uint8_t *)s;
    for (int x = 0; x < count; x += 1) {
        uint8_t val = *(x + p);
        while (val) {
            bits += val & 1;
            val >>= 1;
        }
    }
    return bits;
}

static int test_case(const char *msg, int size) {
    uint8_t buf[size];
    int fuzzing = 1000;
    for (int y = 0; y < fuzzing; y += 1) {
        for (int z = 0; z < size; z += 1) {
            buf[z] = rand() % 256;
        }

        long long expect = bitcount(buf, size);
        long long ret_scalar = popcountScalar(buf, size);
        TEST_ASSERT_MESSAGE(msg, expect == ret_scalar);
#ifdef HAVE_AVX2
        long long ret_avx2 = popcountAVX2(buf, size);
        TEST_ASSERT_MESSAGE(msg, expect == ret_avx2);
#endif
    }

    return 0;
}

int test_popcount(int argc, char **argv, int flags) {
    UNUSED(argc);
    UNUSED(argv);
    UNUSED(flags);

    srand(time(NULL));

#define TEST_CASE(MSG, SIZE)    \
    if (test_case(MSG, SIZE)) { \
        return 1;               \
    }

Review comment (suggestion):
use `random()`  if possible (for quality and range, random() is typically superior, though portability might favor rand())

Code after:
#include "../zmalloc.h"

extern long long popcountScalar(void *s, long count);
#ifdef HAVE_AVX2
extern long long popcountAVX2(void *s, long count);
#endif

static long long bitcount(void *s, long count) {
    long long bits = 0;
    uint8_t *p = (uint8_t *)s;
    for (int x = 0; x < count; x += 1) {
        uint8_t val = *(x + p);
        while (val) {
            bits += val & 1;
            val >>= 1;
        }
    }
    return bits;
}

static int test_case(const char *msg, int size) {
    uint8_t buf[size];
    int fuzzing = 1000;
    for (int y = 0; y < fuzzing; y += 1) {
        for (int z = 0; z < size; z += 1) {
            buf[z] = random() % 256;
        }

        long long expect = bitcount(buf, size);
        long long ret_scalar = popcountScalar(buf, size);
        TEST_ASSERT_MESSAGE(msg, expect == ret_scalar);
#ifdef HAVE_AVX2
        long long ret_avx2 = popcountAVX2(buf, size);
        TEST_ASSERT_MESSAGE(msg, expect == ret_avx2);
#endif
    }

    return 0;
}

int test_popcount(int argc, char **argv, int flags) {
    UNUSED(argc);
    UNUSED(argv);
    UNUSED(flags);

#define TEST_CASE(MSG, SIZE)                    \
    if (test_case("Test failed: " MSG, SIZE)) { \
        return 1;                               \
    }

    /* The AVX2 version divides the array into the following 3 parts."
Repository: valkey-io/valkey
File: src/valkey-benchmark.c (C)

Code before:

static struct config {
    aeEventLoop *el;
    cliConnInfo conn_info;
    const char *hostsocket;
    int tls;
    struct cliSSLconfig sslconfig;
    int numclients;
    _Atomic int liveclients;
    int requests;
    _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 randomkeys;
    int randomkeys_keyspacelen;
    int keepalive;
    int pipeline;
    long long start;
    long long totlatency;
    const char *title;
    list *clients;
    list *pause_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;
    int num_threads;
    struct benchmarkThread **threads;
    int cluster_mode;
    readFromReplica read_from_replica;
    int cluster_node_count;
    struct clusterNode **cluster_nodes;
    struct serverConfig *redis_config;
    struct hdr_histogram *latency_histogram;
    struct hdr_histogram *current_sec_latency_histogram;
    _Atomic int is_fetching_slots;
    _Atomic int is_updating_slots;
    _Atomic int slots_last_update;
    int enable_tracking;
    int num_functions;
    int num_keys_in_fcall;
    pthread_mutex_t liveclients_mutex;
    pthread_mutex_t is_updating_slots_mutex;

Review comment (suggestion):
Client in this list are already paused, so "paused" is better than "pause" I think.

```suggestion
    list *paused_clients;
```

Code after:
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;
    _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 replacekeys;
    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;
    int num_threads;
    struct benchmarkThread **threads;
    int cluster_mode;
    readFromReplica read_from_replica;
    int cluster_node_count;
    struct clusterNode **cluster_nodes;
    struct serverConfig *redis_config;
    struct hdr_histogram *latency_histogram;
    struct hdr_histogram *current_sec_latency_histogram;
    _Atomic int is_fetching_slots;
    _Atomic int is_updating_slots;
    _Atomic int slots_last_update;
    int enable_tracking;
    int num_functions;
    int num_keys_in_fcall;
    pthread_mutex_t liveclients_mutex;
Repository: valkey-io/valkey
File: deps/lua/src/lua_cjson.c (C)

Code before:
{
    if (!cfg->encode_keep_buffer)
        strbuf_free(json);
    luaL_error(l, "Cannot serialise %s: %s",
                  lua_typename(l, lua_type(l, lindex)), reason);
}

/* json_append_string args:
 * - lua_State
 * - JSON strbuf
 * - String (Lua stack index)
 *
 * Returns nothing. Doesn't remove string from Lua stack */
static void json_append_string(lua_State *l, strbuf_t *json, int lindex)
{
    const char *escstr;
    const char *str;
    size_t i, len;

    str = lua_tolstring(l, lindex, &len);

    if (len > SIZE_MAX - 3)
        abort(); /* Overflow check */
    strbuf_ensure_empty_length(json, len + 2);

    strbuf_append_char(json, '\"');
    for (i = 0; i < len; i++) {
        escstr = char2escape[(unsigned char)str[i]];
        if (escstr)
            strbuf_append_string(json, escstr);
        else
            strbuf_append_char(json, str[i]);
    }
    strbuf_append_char(json, '\"');
}

/* Find the size of the array on the top of the Lua stack
 * -1   object (not a pure array)
 * >=0  elements in array
 */
static int lua_array_length(lua_State *l, json_config_t *cfg, strbuf_t *json)
{
    double k;
    int max;
    int items;

    max = 0;
    items = 0;

    lua_pushnil(l);
    /* table, startkey */

Review comment (suggestion):
We can write the first char unsafe, because we have ensured len + 2, so it should be safe.

We can use this to trigger the extra-tests to run after adding the label.

```suggestion
    strbuf_append_char_unsafe(json, '\"');
```

Code after:
{
    if (!cfg->encode_keep_buffer)
        strbuf_free(json);
    luaL_error(l, "Cannot serialise %s: %s",
                  lua_typename(l, lua_type(l, lindex)), reason);
}

/* json_append_string args:
 * - lua_State
 * - JSON strbuf
 * - String (Lua stack index)
 *
 * Returns nothing. Doesn't remove string from Lua stack */
static void json_append_string(lua_State *l, strbuf_t *json, int lindex)
{
    const char *escstr;
    const char *str;
    size_t i, len;

    str = lua_tolstring(l, lindex, &len);

    if (len > SIZE_MAX - 3)
        abort(); /* Overflow check */
    strbuf_ensure_empty_length(json, len + 2);

    strbuf_append_char_unsafe(json, '\"');
    for (i = 0; i < len; i++) {
        escstr = char2escape[(unsigned char)str[i]];
        if (escstr)
            strbuf_append_string(json, escstr);
        else
            strbuf_append_char(json, str[i]);
    }
    strbuf_append_char(json, '\"');
}

/* Find the size of the array on the top of the Lua stack
 * -1   object (not a pure array)
 * >=0  elements in array
 */
static int lua_array_length(lua_State *l, json_config_t *cfg, strbuf_t *json)
{
    double k;
    int max;
    int items;

    max = 0;
    items = 0;

    lua_pushnil(l);
    /* table, startkey */
Repository: valkey-io/valkey
File: src/adlist.c (C)

Code before:
    }
    if (node->prev != NULL) {
        node->prev->next = node;
    }
    if (node->next != NULL) {
        node->next->prev = node;
    }
    list->len++;
    return list;
}

/* Remove the specified node from the specified list.
 * The node is freed. If free callback is provided the value is freed as well.
 *
 * This function can't fail. */
void listDelNode(list *list, listNode *node) {
    listUnlinkNode(list, node);
    if (list->free) list->free(node->value);
    zfree(node);
}

/*
 * Remove the specified node from the list without freeing it.
 */
void listUnlinkNode(list *list, listNode *node) {
    serverAssert(list->len > 0);
    if (node->prev)
        node->prev->next = node->next;
    else
        list->head = node->next;
    if (node->next)
        node->next->prev = node->prev;
    else
        list->tail = node->prev;

    node->next = NULL;
    node->prev = NULL;

    list->len--;
}

/* Returns a list iterator 'iter'. After the initialization every
 * call to listNext() will return the next element of the list.
 *
 * This function can't fail. */
listIter *listGetIterator(list *list, int direction) {
    listIter *iter;

    if ((iter = zmalloc(sizeof(*iter))) == NULL) return NULL;
    if (direction == AL_START_HEAD)
        iter->next = list->head;

Review comment (suggestion):
It's just `assert` when we use `serverassert.h`.

```suggestion
    assert(list->len > 0);
```

Code after:
    }
    if (node->prev != NULL) {
        node->prev->next = node;
    }
    if (node->next != NULL) {
        node->next->prev = node;
    }
    list->len++;
    return list;
}

/* Remove the specified node from the specified list.
 * The node is freed. If free callback is provided the value is freed as well.
 *
 * This function can't fail. */
void listDelNode(list *list, listNode *node) {
    listUnlinkNode(list, node);
    if (list->free) list->free(node->value);
    zfree(node);
}

/*
 * Remove the specified node from the list without freeing it.
 */
void listUnlinkNode(list *list, listNode *node) {
    assert(list->len > 0);

    if (node->prev) {
        assert(node->prev->next == node);
        node->prev->next = node->next;
    } else {
        assert(list->head == node);
        list->head = node->next;
    }

    if (node->next) {
        assert(node->next->prev == node);
        node->next->prev = node->prev;
    } else {
        assert(list->tail == node);
        list->tail = node->prev;
    }

    node->next = NULL;
    node->prev = NULL;

    list->len--;
}

/* Returns a list iterator 'iter'. After the initialization every
 * call to listNext() will return the next element of the list.
Repository: valkey-io/valkey
File: src/anet.c (C)

Code before:
 * POSSIBILITY OF SUCH DAMAGE.
 */

#include "fmacros.h"

#include <sys/types.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/un.h>
#include <sys/time.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <arpa/inet.h>
#include <unistd.h>
#include <fcntl.h>
#include <string.h>
#include <netdb.h>
#include <errno.h>
#include <stdarg.h>
#include <stdio.h>
#include <grp.h>

#include "anet.h"
#include "config.h"
#include "util.h"
#include "server.h"

static void anetSetError(char *err, const char *fmt, ...) {
    va_list ap;

    if (!err) return;
    va_start(ap, fmt);
    vsnprintf(err, ANET_ERR_LEN, fmt, ap);
    va_end(ap);
}

int anetGetError(int fd) {
    int sockerr = 0;
    socklen_t errlen = sizeof(sockerr);

    if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &sockerr, &errlen) == -1) sockerr = errno;
    return sockerr;
}

static int anetGetSocketFlags(char *err, int fd) {
    int flags;

    if ((flags = fcntl(fd, F_GETFL)) == -1) {
        anetSetError(err, "fcntl(F_GETFL): %s", strerror(errno));
        return ANET_ERR;
    }

Review comment (suggestion):
Oh.. I didn't realize this implication. To keep some parts less coupled with the huge server.h which essentially makes everything depend on everything, we usually try to avoid including it when possible. There is a smaller header file `serverassert.h` that provides an `assert` macro that we can use instead. It has the same effect, but it avoids the coupling.

Code after:
 * POSSIBILITY OF SUCH DAMAGE.
 */

#include "fmacros.h"

#include <sys/types.h>
#include <sys/socket.h>
#include <sys/stat.h>
#include <sys/un.h>
#include <sys/time.h>
#include <netinet/in.h>
#include <netinet/tcp.h>
#include <arpa/inet.h>
#include <unistd.h>
#include <fcntl.h>
#include <string.h>
#include <netdb.h>
#include <errno.h>
#include <stdarg.h>
#include <stdio.h>
#include <grp.h>

#include "anet.h"
#include "config.h"
#include "util.h"
#include "serverassert.h"

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

static void anetSetError(char *err, const char *fmt, ...) {
    va_list ap;

    if (!err) return;
    va_start(ap, fmt);
    vsnprintf(err, ANET_ERR_LEN, fmt, ap);
    va_end(ap);
}

int anetGetError(int fd) {
    int sockerr = 0;
    socklen_t errlen = sizeof(sockerr);

    if (getsockopt(fd, SOL_SOCKET, SO_ERROR, &sockerr, &errlen) == -1) sockerr = errno;
    return sockerr;
}

static int anetGetSocketFlags(char *err, int fd) {
    int flags;

    if ((flags = fcntl(fd, F_GETFL)) == -1) {
        anetSetError(err, "fcntl(F_GETFL): %s", strerror(errno));
Repository: valkey-io/valkey
File: src/hyperloglog.c (C)

Code before:
}

/* Different debugging related operations about the HLL implementation.
 *
 * PFDEBUG GETREG <key>
 * PFDEBUG DECODE <key>
 * PFDEBUG ENCODING <key>
 * PFDEBUG TODENSE <key>
 * PFDEBUG SIMD (ON|OFF)
 */
void pfdebugCommand(client *c) {
    char *cmd = c->argv[1]->ptr;
    struct hllhdr *hdr;
    robj *o;
    int j;

    if (!strcasecmp(cmd, "simd")) {
        if (c->argc != 3) goto arityerr;

        if (!strcasecmp(c->argv[2]->ptr, "on")) {
#ifdef HAVE_AVX2
            simd_enabled = 1;
#endif
#ifdef __ARM_NEON
            simd_enabled = 1;
#endif
        } else if (!strcasecmp(c->argv[2]->ptr, "off")) {
#ifdef HAVE_AVX2
            simd_enabled = 0;
#endif
#ifdef __ARM_NEON
            simd_enabled = 0;
#endif
        } else {
            addReplyError(c, "Argument must be ON or OFF");
        }

        if (HLL_USE_AVX2 || HLL_USE_NEON) {
            addReplyStatus(c, "enabled");
        } else {
            addReplyStatus(c, "disabled");
        }

        return;
    }

    o = lookupKeyWrite(c->db, c->argv[2]);
    if (o == NULL) {
        addReplyError(c, "The specified key does not exist");
        return;
    }

Review comment (suggestion):
    #if defined(HAVE_AVX) || defined(__ARM_NEON)

or we can consider some macro like `HLL_HAVE_SIMD` to avoid this repetition, also to avoid defining the variable twice.

Code after:
    /* Success! */
    addReply(c, shared.ok);

cleanup:
    sdsfree(bitcounters);
    if (o) decrRefCount(o);
}

/* Different debugging related operations about the HLL implementation.
 *
 * PFDEBUG GETREG <key>
 * PFDEBUG DECODE <key>
 * PFDEBUG ENCODING <key>
 * PFDEBUG TODENSE <key>
 * PFDEBUG SIMD (ON|OFF)
 */
void pfdebugCommand(client *c) {
    char *cmd = c->argv[1]->ptr;
    struct hllhdr *hdr;
    robj *o;
    int j;

    if (!strcasecmp(cmd, "simd")) {
        if (c->argc != 3) goto arityerr;

        if (!strcasecmp(c->argv[2]->ptr, "on")) {
#if SIMD_SUPPORTED
            simd_enabled = 1;
#endif
        } else if (!strcasecmp(c->argv[2]->ptr, "off")) {
#if SIMD_SUPPORTED
            simd_enabled = 0;
#endif
        } else {
            addReplyError(c, "Argument must be ON or OFF");
        }

        addReplyStatus(c, (HLL_USE_AVX2 || HLL_USE_NEON) ? "enabled" : "disabled");

        return;
    }

    o = lookupKeyWrite(c->db, c->argv[2]);
    if (o == NULL) {
        addReplyError(c, "The specified key does not exist");
        return;
    }
    if (isHLLObjectOrReply(c, o) != C_OK) return;
    o = dbUnshareStringValue(c->db, c->argv[2], o);
    hdr = o->ptr;
Repository: valkey-io/valkey
File: src/config.c (C)

Code before:
    {NULL, 0}};

configEnum shutdown_on_sig_enum[] = {
    {"default", 0},
    {"save", SHUTDOWN_SAVE},
    {"nosave", SHUTDOWN_NOSAVE},
    {"now", SHUTDOWN_NOW},
    {"force", SHUTDOWN_FORCE},
    {NULL, 0}};

configEnum repl_diskless_load_enum[] = {
    {"disabled", REPL_DISKLESS_LOAD_DISABLED},
    {"on-empty-db", REPL_DISKLESS_LOAD_WHEN_DB_EMPTY},
    {"swapdb", REPL_DISKLESS_LOAD_SWAPDB},
    {"flush-before-load", REPL_DISKLESS_LOAD_FLUSH_BEFORE_LOAD},
    {NULL, 0}};

configEnum tls_auth_clients_enum[] = {
    {"no", TLS_CLIENT_AUTH_NO},
    {"yes", TLS_CLIENT_AUTH_YES},
    {"optional", TLS_CLIENT_AUTH_OPTIONAL},
    {NULL, 0}};

configEnum tls_client_auth_user_enum[] = {
    {"CN", TLS_CLIENT_FIELD_CN},
    {"O", TLS_CLIENT_FIELD_O},
    {NULL, 0} // terminator
};

configEnum oom_score_adj_enum[] = {
    {"no", OOM_SCORE_ADJ_NO},
    {"yes", OOM_SCORE_RELATIVE},
    {"relative", OOM_SCORE_RELATIVE},
    {"absolute", OOM_SCORE_ADJ_ABSOLUTE},
    {NULL, 0}};

configEnum acl_pubsub_default_enum[] = {
    {"allchannels", SELECTOR_FLAG_ALLCHANNELS},
    {"resetchannels", 0},
    {NULL, 0}};

configEnum sanitize_dump_payload_enum[] = {
    {"no", SANITIZE_DUMP_NO},
    {"yes", SANITIZE_DUMP_YES},
    {"clients", SANITIZE_DUMP_CLIENTS},
    {NULL, 0}};

configEnum protected_action_enum[] = {
    {"no", PROTECTED_ACTION_ALLOWED_NO},
    {"yes", PROTECTED_ACTION_ALLOWED_YES},
    {"local", PROTECTED_ACTION_ALLOWED_LOCAL},

Review comment (suggestion):
Latest from the weekly meeting is leave this as an enum but only support `CN` and `off` initially. We can add `O` or something else later as needed.

Code after:
    {"no", AOF_FSYNC_NO},
    {NULL, 0}};

configEnum shutdown_on_sig_enum[] = {
    {"default", 0},
    {"save", SHUTDOWN_SAVE},
    {"nosave", SHUTDOWN_NOSAVE},
    {"now", SHUTDOWN_NOW},
    {"force", SHUTDOWN_FORCE},
    {NULL, 0}};

configEnum repl_diskless_load_enum[] = {
    {"disabled", REPL_DISKLESS_LOAD_DISABLED},
    {"on-empty-db", REPL_DISKLESS_LOAD_WHEN_DB_EMPTY},
    {"swapdb", REPL_DISKLESS_LOAD_SWAPDB},
    {"flush-before-load", REPL_DISKLESS_LOAD_FLUSH_BEFORE_LOAD},
    {NULL, 0}};

configEnum tls_auth_clients_enum[] = {
    {"no", TLS_CLIENT_AUTH_NO},
    {"yes", TLS_CLIENT_AUTH_YES},
    {"optional", TLS_CLIENT_AUTH_OPTIONAL},
    {NULL, 0}};

configEnum tls_client_auth_user_enum[] = {
    {"CN", TLS_CLIENT_FIELD_CN},
    {"off", TLS_CLIENT_FIELD_OFF},
    {NULL, 0} // terminator
};

configEnum oom_score_adj_enum[] = {
    {"no", OOM_SCORE_ADJ_NO},
    {"yes", OOM_SCORE_RELATIVE},
    {"relative", OOM_SCORE_RELATIVE},
    {"absolute", OOM_SCORE_ADJ_ABSOLUTE},
    {NULL, 0}};

configEnum acl_pubsub_default_enum[] = {
    {"allchannels", SELECTOR_FLAG_ALLCHANNELS},
    {"resetchannels", 0},
    {NULL, 0}};

configEnum sanitize_dump_payload_enum[] = {
    {"no", SANITIZE_DUMP_NO},
    {"yes", SANITIZE_DUMP_YES},
    {"clients", SANITIZE_DUMP_CLIENTS},
    {NULL, 0}};

configEnum protected_action_enum[] = {
    {"no", PROTECTED_ACTION_ALLOWED_NO},
    {"yes", PROTECTED_ACTION_ALLOWED_YES},