ronantakizawa/codereview-bench huggingface dataset:
Repository: nomic-ai/gpt4all
File: gpt4all-chat/database.cpp (C++)

Code before:
        qDebug() << "Error selecting chunks:" << q.lastError().text();
        return q.lastError();
    }
#endif

    return QSqlError();
}

Database::Database(int chunkSize)
    : QObject(nullptr)
    , m_chunkSize(chunkSize)
    , m_scanTimer(new QTimer(this))
    , m_watcher(new QFileSystemWatcher(this))
    , m_embLLM(new EmbeddingLLM)
    , m_embeddings(new Embeddings(this))
{
    moveToThread(&m_dbThread);
    m_dbThread.setObjectName("database");
    m_dbThread.start();
}

Database::~Database()
{
    delete m_embLLM;
    m_dbThread.quit();
    m_dbThread.wait();
}

void Database::scheduleNext(int folder_id, size_t countForFolder)
{
    emit updateCurrentDocsToIndex(folder_id, countForFolder);
    if (!countForFolder) {
        updateFolderStatus(folder_id, FolderStatus::Complete);
        emit updateInstalled(folder_id, true);
    }
    if (m_docsToScan.isEmpty()) {
        m_scanTimer->stop();
        updateIndexingStatus();
    }
}

void Database::handleDocumentError(const QString &errorMessage,
    int document_id, const QString &document_path, const QSqlError &error)
{
    qWarning() << errorMessage << document_id << document_path << error.text();
}

size_t Database::chunkStream(QTextStream &stream, int folder_id, int document_id, const QString &file,
    const QString &title, const QString &author, const QString &subject, const QString &keywords, int page,
    int maxChunks)
{

Review comment (suggestion):
There's a potential for a use-after-free if you delete m_embLLM while it is still being used by the database thread.
```suggestion
    m_dbThread.quit();
    m_dbThread.wait();
    delete m_embLLM;
```

Code after:
        qDebug() << "Error selecting chunks:" << q.lastError().text();
        return q.lastError();
    }
#endif

    return QSqlError();
}

Database::Database(int chunkSize)
    : QObject(nullptr)
    , m_chunkSize(chunkSize)
    , m_scanTimer(new QTimer(this))
    , m_watcher(new QFileSystemWatcher(this))
    , m_embLLM(new EmbeddingLLM)
    , m_embeddings(new Embeddings(this))
{
    moveToThread(&m_dbThread);
    m_dbThread.setObjectName("database");
    m_dbThread.start();
}

Database::~Database()
{
    m_dbThread.quit();
    m_dbThread.wait();
    delete m_embLLM;
}

void Database::scheduleNext(int folder_id, size_t countForFolder)
{
    emit updateCurrentDocsToIndex(folder_id, countForFolder);
    if (!countForFolder) {
        updateFolderStatus(folder_id, FolderStatus::Complete);
        emit updateInstalled(folder_id, true);
    }
    if (m_docsToScan.isEmpty()) {
        m_scanTimer->stop();
        updateIndexingStatus();
    }
}

void Database::handleDocumentError(const QString &errorMessage,
    int document_id, const QString &document_path, const QSqlError &error)
{
    qWarning() << errorMessage << document_id << document_path << error.text();
}

size_t Database::chunkStream(QTextStream &stream, int folder_id, int document_id, const QString &file,
    const QString &title, const QString &author, const QString &subject, const QString &keywords, int page,
    int maxChunks)
{
Repository: nomic-ai/gpt4all
File: gpt4all-backend/llamamodel.cpp (C++)

Code before:
    if (!ctx) {
        std::cerr << __func__ << ": gguf_init_from_file failed\n";
        return nullptr;
    }

    int gguf_ver = gguf_get_version(ctx);
    if (gguf_ver > GGUF_VER_MAX) {
        std::cerr << __func__ << ": unsupported gguf version: " << gguf_ver << "\n";
        gguf_free(ctx);
        return nullptr;
    }

    return ctx;
}

static int32_t get_arch_key_u32(std::string const &modelPath, std::string const &archKey) {
    auto * ctx = load_gguf(modelPath.c_str());
    if (!ctx)
        return -1;
    std::string arch = get_arch_name(ctx);

    int32_t value = -1;
    if (ctx) {
        auto key = arch + "." + archKey;
        int keyidx = gguf_find_key(ctx, key.c_str());
        if (keyidx != -1) {
            value = gguf_get_val_u32(ctx, keyidx);
        } else {
            std::cerr << __func__ << ": " << key << "not found in " << modelPath << "\n";
        }
    }

    gguf_free(ctx);
    return value;
}

struct LLamaPrivate {
    const std::string modelPath;
    bool modelLoaded = false;
    int device = -1;
    llama_model *model = nullptr;
    llama_context *ctx = nullptr;
    llama_model_params model_params;
    llama_context_params ctx_params;
    int64_t n_threads = 0;
    std::vector<LLModel::Token> end_tokens;
    const char *backend_name = nullptr;
};

LLamaModel::LLamaModel()
    : d_ptr(new LLamaPrivate) {}

Review comment (suggestion):
In general, I prefer for one line conditionals not to have braces unless it aids with code readability. In general, try and err on the side of not including the braces if they don't help and refrain from adding braces if they don't help.

Code after:
    };
    gguf_context *ctx = gguf_init_from_file(fname, params);
    if (!ctx) {
        std::cerr << __func__ << ": gguf_init_from_file failed\n";
        return nullptr;
    }

    int gguf_ver = gguf_get_version(ctx);
    if (gguf_ver > GGUF_VER_MAX) {
        std::cerr << __func__ << ": unsupported gguf version: " << gguf_ver << "\n";
        gguf_free(ctx);
        return nullptr;
    }

    return ctx;
}

static int32_t get_arch_key_u32(std::string const &modelPath, std::string const &archKey) {
    int32_t value = -1;
    std::string arch;

    auto * ctx = load_gguf(modelPath.c_str());
    if (!ctx)
        goto cleanup;

    try {
        arch = get_arch_name(ctx);
    } catch (const std::runtime_error &) {
        goto cleanup; // cannot read key
    }

    {
        auto key = arch + "." + archKey;
        int keyidx = gguf_find_key(ctx, key.c_str());
        if (keyidx != -1) {
            value = gguf_get_val_u32(ctx, keyidx);
        } else {
            std::cerr << __func__ << ": " << key << "not found in " << modelPath << "\n";
        }
    }

cleanup:
    gguf_free(ctx);
    return value;
}

struct LLamaPrivate {
    const std::string modelPath;
    bool modelLoaded = false;
    int device = -1;
    llama_model *model = nullptr;
Repository: nomic-ai/gpt4all
File: gpt4all-backend/llamamodel.cpp (C++)

Code before:
            && n_vocab == 32002
            && gguf_get_arr_str(ctx, token_idx, 32000) == "<dummy32000>"s // should be 

        ) {
            res = true;
        }
    } catch (const std::logic_error &e) {
        std::cerr << __func__ << ": " << e.what() << "\n";
    }

    gguf_free(ctx);
    return res;
}

bool LLamaModel::isEmbeddingModel(const std::string &modelPath) const {
    auto *ctx_gguf = load_gguf(modelPath.c_str());
    if (!ctx_gguf) {
        std::cerr << __func__ << ": failed to load GGUF from " <<  modelPath << "\n";
        return false;
    }

    std::string arch = get_arch_name(ctx_gguf);
    gguf_free(ctx_gguf);
    return is_embedding_arch(arch);
}

bool LLamaModel::loadModel(const std::string &modelPath, int n_ctx, int ngl)
{
    d_ptr->modelLoaded = false;

    // clean up after previous loadModel()
    if (d_ptr->model) {
        llama_free_model(d_ptr->model);
        d_ptr->model = nullptr;
    }
    if (d_ptr->ctx) {
        llama_free(d_ptr->ctx);
        d_ptr->ctx = nullptr;
    }

    if (n_ctx < 8) {
        std::cerr << "warning: minimum context size is 8, using minimum size.\n";
        n_ctx = 8;
    }

    // -- load the model --

    gpt_params params;

    d_ptr->model_params = llama_model_default_params();

    d_ptr->model_params.use_mmap  = params.use_mmap;

Review comment (suggestion):
I prefer this to be closer to where it is actually defined right before the try statement. This is C style where it is at the top, but makes it harder to read. Better to put it closer to the scope it is introduced.

Code after:
    };

    bool res = false;
    try {
        std::string name(gguf_get_val_str(ctx, get_key("general.name")));
        int token_idx = get_key("tokenizer.ggml.tokens");
        int n_vocab = gguf_get_arr_n(ctx, token_idx);

        // check for known bad models
        if (name == "open-orca_mistral-7b-openorca"
            && n_vocab == 32002
            && gguf_get_arr_str(ctx, token_idx, 32000) == "<dummy32000>"s // should be 

        ) {
            res = true;
        }
    } catch (const std::logic_error &e) {
        std::cerr << __func__ << ": " << e.what() << "\n";
    }

    gguf_free(ctx);
    return res;
}

bool LLamaModel::isEmbeddingModel(const std::string &modelPath) const {
    bool result = false;
    std::string arch;

    auto *ctx_gguf = load_gguf(modelPath.c_str());
    if (!ctx_gguf) {
        std::cerr << __func__ << ": failed to load GGUF from " <<  modelPath << "\n";
        goto cleanup;
    }

    try {
        arch = get_arch_name(ctx_gguf);
    } catch (const std::runtime_error &) {
        goto cleanup; // cannot read key
    }

    result = is_embedding_arch(arch);

cleanup:
    gguf_free(ctx_gguf);
    return result;
}

bool LLamaModel::loadModel(const std::string &modelPath, int n_ctx, int ngl)
{
    d_ptr->modelLoaded = false;

    // clean up after previous loadModel()
Repository: nomic-ai/gpt4all
File: gpt4all-chat/src/chatmodel.h (C/C++)

Code before:
    int id = 0;
    QString name;
    QString value;
    QString newResponse;
    QList<ResultInfo> sources;
    QList<ResultInfo> consolidatedSources;
    bool currentResponse = false;
    bool stopped = false;
    bool thumbsUpState = false;
    bool thumbsDownState = false;
};
Q_DECLARE_METATYPE(ChatItem)

class ChatModel;
class ChatModelIterator
{
public:
    ChatItem &operator*() const;
    ChatModelIterator &operator++();
    ChatModelIterator operator++(int);
    bool operator<(const ChatModelIterator& other) const;

private:
    ChatModelIterator(ChatModel* model, int index);
    friend class ChatModel;
    ChatModel* m_model;
    int m_index;
};

class ChatModel : public QAbstractListModel
{
    Q_OBJECT
    Q_PROPERTY(int count READ count NOTIFY countChanged)

public:
    explicit ChatModel(QObject *parent = nullptr) : QAbstractListModel(parent) {}

    enum Roles {
        IdRole = Qt::UserRole + 1,
        NameRole,
        ValueRole,
        NewResponseRole,
        CurrentResponseRole,
        StoppedRole,
        ThumbsUpStateRole,
        ThumbsDownStateRole,
        SourcesRole,
        ConsolidatedSourcesRole
    };

    int rowCount(const QModelIndex &parent = QModelIndex()) const override

Review comment (suggestion):
```suggestion
    bool operator<(const ChatModelIterator &other) const;

private:
    ChatModelIterator(ChatModel *model, int index);
    friend class ChatModel;
    ChatModel *m_model;
```

Code after:
    int id = 0;
    QString name;
    QString value;
    QString newResponse;
    QList<ResultInfo> sources;
    QList<ResultInfo> consolidatedSources;
    bool currentResponse = false;
    bool stopped = false;
    bool thumbsUpState = false;
    bool thumbsDownState = false;
};
Q_DECLARE_METATYPE(ChatItem)

using ChatModelIterator = QList<ChatItem>::const_iterator;

class ChatModel : public QAbstractListModel
{
    Q_OBJECT
    Q_PROPERTY(int count READ count NOTIFY countChanged)

public:
    explicit ChatModel(QObject *parent = nullptr) : QAbstractListModel(parent) {}

    enum Roles {
        IdRole = Qt::UserRole + 1,
        NameRole,
        ValueRole,
        NewResponseRole,
        CurrentResponseRole,
        StoppedRole,
        ThumbsUpStateRole,
        ThumbsDownStateRole,
        SourcesRole,
        ConsolidatedSourcesRole
    };

    int rowCount(const QModelIndex &parent = QModelIndex()) const override
    {
        QMutexLocker locker(&m_mutex);
        Q_UNUSED(parent)
        return m_chatItems.size();
    }

    QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override
    {
        QMutexLocker locker(&m_mutex);
        if (!index.isValid() || index.row() < 0 || index.row() >= m_chatItems.size())
            return QVariant();

        const ChatItem &item = m_chatItems.at(index.row());
        switch (role) {
Repository: nomic-ai/gpt4all
File: gpt4all-chat/src/chatmodel.h (C/C++)

Code before:
    QString name;
    QString value;
    QString newResponse;
    QList<ResultInfo> sources;
    QList<ResultInfo> consolidatedSources;
    bool currentResponse = false;
    bool stopped = false;
    bool thumbsUpState = false;
    bool thumbsDownState = false;
};
Q_DECLARE_METATYPE(ChatItem)

class ChatModel;
class ChatModelIterator
{
public:
    ChatItem &operator*() const;
    ChatModelIterator &operator++();
    ChatModelIterator operator++(int);
    bool operator<(const ChatModelIterator& other) const;

private:
    ChatModelIterator(ChatModel* model, int index);
    friend class ChatModel;
    ChatModel* m_model;
    int m_index;
};

class ChatModel : public QAbstractListModel
{
    Q_OBJECT
    Q_PROPERTY(int count READ count NOTIFY countChanged)

public:
    explicit ChatModel(QObject *parent = nullptr) : QAbstractListModel(parent) {}

    enum Roles {
        IdRole = Qt::UserRole + 1,
        NameRole,
        ValueRole,
        NewResponseRole,
        CurrentResponseRole,
        StoppedRole,
        ThumbsUpStateRole,
        ThumbsDownStateRole,
        SourcesRole,
        ConsolidatedSourcesRole
    };

    int rowCount(const QModelIndex &parent = QModelIndex()) const override
    {

Review comment (suggestion):
This could be:
```suggestion
    typename QList<ChatItem>::iterator m_iter;
```
See above.

Code after:
    QString name;
    QString value;
    QString newResponse;
    QList<ResultInfo> sources;
    QList<ResultInfo> consolidatedSources;
    bool currentResponse = false;
    bool stopped = false;
    bool thumbsUpState = false;
    bool thumbsDownState = false;
};
Q_DECLARE_METATYPE(ChatItem)

using ChatModelIterator = QList<ChatItem>::const_iterator;

class ChatModel : public QAbstractListModel
{
    Q_OBJECT
    Q_PROPERTY(int count READ count NOTIFY countChanged)

public:
    explicit ChatModel(QObject *parent = nullptr) : QAbstractListModel(parent) {}

    enum Roles {
        IdRole = Qt::UserRole + 1,
        NameRole,
        ValueRole,
        NewResponseRole,
        CurrentResponseRole,
        StoppedRole,
        ThumbsUpStateRole,
        ThumbsDownStateRole,
        SourcesRole,
        ConsolidatedSourcesRole
    };

    int rowCount(const QModelIndex &parent = QModelIndex()) const override
    {
        QMutexLocker locker(&m_mutex);
        Q_UNUSED(parent)
        return m_chatItems.size();
    }

    QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override
    {
        QMutexLocker locker(&m_mutex);
        if (!index.isValid() || index.row() < 0 || index.row() >= m_chatItems.size())
            return QVariant();

        const ChatItem &item = m_chatItems.at(index.row());
        switch (role) {
            case IdRole:
Repository: nomic-ai/gpt4all
File: gpt4all-chat/src/chat.h (C/C++)

Code before:
    QString m_generatedName;
    QString m_userName;
    ModelInfo m_modelInfo;
    QString m_modelLoadingError;
    QString m_tokenSpeed;
    QString m_device;
    QString m_fallbackReason;
    QString m_response;
    QList<QString> m_collections;
    QList<QString> m_generatedQuestions;
    ChatModel *m_chatModel;
    bool m_responseInProgress = false;
    ResponseState m_responseState;
    qint64 m_creationDate;
    ChatLLM *m_llmodel;
    QList<ResultInfo> m_databaseResults;
    bool m_isServer = false;
    bool m_shouldDeleteLater = false;
    float m_modelLoadingPercentage = 0.0f;
    LocalDocsCollectionsModel *m_collectionModel;
    bool m_firstResponse = true;
    int m_trySwitchContextInProgress = 0;
    bool m_isCurrentlyLoading = false;
    // True if we need to serialize the chat to disk, because of one of two reasons:
    // - The chat was freshly created during this launch.
    // - The chat was interacted with after loading it from disk.
    bool m_needsSave = true;
};

#endif // CHAT_H


Review comment (suggestion):
```suggestion
    // - The chat was changed after loading it from disk.
```

Code after:
    QString m_generatedName;
    QString m_userName;
    ModelInfo m_modelInfo;
    QString m_modelLoadingError;
    QString m_tokenSpeed;
    QString m_device;
    QString m_fallbackReason;
    QString m_response;
    QList<QString> m_collections;
    QList<QString> m_generatedQuestions;
    ChatModel *m_chatModel;
    bool m_responseInProgress = false;
    ResponseState m_responseState;
    qint64 m_creationDate;
    ChatLLM *m_llmodel;
    QList<ResultInfo> m_databaseResults;
    bool m_isServer = false;
    bool m_shouldDeleteLater = false;
    float m_modelLoadingPercentage = 0.0f;
    LocalDocsCollectionsModel *m_collectionModel;
    bool m_firstResponse = true;
    int m_trySwitchContextInProgress = 0;
    bool m_isCurrentlyLoading = false;
    // True if we need to serialize the chat to disk, because of one of two reasons:
    // - The chat was freshly created during this launch.
    // - The chat was changed after loading it from disk.
    bool m_needsSave = true;
};

#endif // CHAT_H
Repository: nomic-ai/gpt4all
File: gpt4all-chat/src/modellist.cpp (C++)

Code before:
#include <utility>

using namespace Qt::Literals::StringLiterals;

//#define USE_LOCAL_MODELSJSON

#define MODELS_JSON_VERSION "3"


static const QStringList FILENAME_BLACKLIST { u"gpt4all-nomic-embed-text-v1.rmodel"_s };

static const QString RMODEL_CHAT_TEMPLATE = uR"(<chat>
{%- set loop_messages = messages %}
{%- for message in loop_messages %}
    {%- if not message['role'] in ['user', 'assistant', 'system'] %}
        {{- raise_exception('Unknown role: ' + message['role']) }}
    {%- endif %}
    {{- '<' + message['role'] + '>' }}
    {%- if message['role'] == 'user' %}
        {%- for source in message.sources %}
            {%- if loop.first %}
                {{- '### Context:\n' }}
            {%- endif %}
            {{- ('Collection: ' + source.collection + '\n'    +
                 'Path: '       + source.path       + '\n'    +
                 'Excerpt: '    + source.text       + '\n\n') - escape }}
        {%- endfor %}
    {%- endif %}
    {%- for attachment in message.prompt_attachments %}
        {{- (attachment.processed_content + '\n\n') | escape }}
    {%- endfor %}
    {{- message.content | escape }}
    {{- '</' + message['role'] + '>' }}
{%- endfor %}
</chat>)"_s;


QString ModelInfo::id() const
{
    return m_id;
}

void ModelInfo::setId(const QString &id)
{
    m_id = id;
}

QString ModelInfo::name() const
{
    return MySettings::globalInstance()->modelName(*this);
}

Review comment (security):
typo
```suggestion
                 'Excerpt: '    + source.text       + '\n\n') | escape }}
```

Code after:
#include <utility>

using namespace Qt::Literals::StringLiterals;

//#define USE_LOCAL_MODELSJSON

#define MODELS_JSON_VERSION "3"


static const QStringList FILENAME_BLACKLIST { u"gpt4all-nomic-embed-text-v1.rmodel"_s };

static const QString RMODEL_CHAT_TEMPLATE = uR"(<chat>
{%- set loop_messages = messages %}
{%- for message in loop_messages %}
    {%- if not message['role'] in ['user', 'assistant', 'system'] %}
        {{- raise_exception('Unknown role: ' + message['role']) }}
    {%- endif %}
    {{- '<' + message['role'] + '>' }}
    {%- if message['role'] == 'user' %}
        {%- for source in message.sources %}
            {%- if loop.first %}
                {{- '### Context:\n' }}
            {%- endif %}
            {{- ('Collection: ' + source.collection + '\n'    +
                 'Path: '       + source.path       + '\n'    +
                 'Excerpt: '    + source.text       + '\n\n') | escape }}
        {%- endfor %}
    {%- endif %}
    {%- for attachment in message.prompt_attachments %}
        {{- (attachment.processed_content + '\n\n') | escape }}
    {%- endfor %}
    {{- message.content | escape }}
    {{- '</' + message['role'] + '>' }}
{%- endfor %}
</chat>)"_s;


QString ModelInfo::id() const
{
    return m_id;
}

void ModelInfo::setId(const QString &id)
{
    m_id = id;
}

QString ModelInfo::name() const
{
    return MySettings::globalInstance()->modelName(*this);
}
Repository: oatpp/oatpp
File: src/oatpp/core/async/worker/IOWorker.cpp (C++)

Code before:

  if(blockToConsume) {

    std::unique_lock<oatpp::concurrency::SpinLock> lock(m_backlogLock);
    while (m_backlog.first == nullptr && m_running) {
      m_backlogCondition.wait(lock);
    }
    oatpp::collection::FastQueue<CoroutineHandle>::moveAll(m_backlog, m_queue);
  } else {

    std::unique_lock<oatpp::concurrency::SpinLock> lock(m_backlogLock, std::try_to_lock);
    if (lock.owns_lock()) {
      oatpp::collection::FastQueue<CoroutineHandle>::moveAll(m_backlog, m_queue);
    }

  }

}

void IOWorker::run() {

  v_int32 consumeIteration = 0;
  v_int32 roundIteration = 0;

  v_int64 tick;
  {
    std::chrono::microseconds ms = std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::system_clock::now().time_since_epoch());
    tick = ms.count();
  }

  while(m_running) {

    auto CP = m_queue.first;
    if(CP != nullptr) {

      Action action = CP->iterate();
      auto& schA = getCoroutineScheduledAction(CP);

      switch(action.getType()) {

        case Action::TYPE_IO_REPEAT:

          dismissAction(schA);

          ++ roundIteration;
          if(roundIteration == 10) {
            roundIteration = 0;
            m_queue.round();
          }
          break;


Review comment (suggestion):
What if we just assign directly to `tick` without `ms` var.
```cpp
tick = std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::system_clock::now().time_since_epoch()).count();
```

Creating a separate block may look confusing.

Code after:

  if(blockToConsume) {

    std::unique_lock<oatpp::concurrency::SpinLock> lock(m_backlogLock);
    while (m_backlog.first == nullptr && m_running) {
      m_backlogCondition.wait(lock);
    }
    oatpp::collection::FastQueue<CoroutineHandle>::moveAll(m_backlog, m_queue);
  } else {

    std::unique_lock<oatpp::concurrency::SpinLock> lock(m_backlogLock, std::try_to_lock);
    if (lock.owns_lock()) {
      oatpp::collection::FastQueue<CoroutineHandle>::moveAll(m_backlog, m_queue);
    }

  }

}

void IOWorker::run() {

  v_int32 consumeIteration = 0;
  v_int32 roundIteration = 0;

  v_int64 tick =std::chrono::duration_cast<std::chrono::microseconds>(std::chrono::system_clock::now().time_since_epoch()).count();

  while(m_running) {

    auto CP = m_queue.first;
    if(CP != nullptr) {

      Action action = CP->iterate();
      auto& schA = getCoroutineScheduledAction(CP);

      switch(action.getType()) {

        case Action::TYPE_IO_REPEAT:

          dismissAction(schA);

          ++ roundIteration;
          if(roundIteration == 10) {
            roundIteration = 0;
            m_queue.round();
          }
          break;

          //        case Action::TYPE_IO_WAIT:
//          roundIteration = 0;
//          m_queue.round();
//          break;