ronantakizawa/codereview-bench huggingface dataset:
Repository: skyline-emu/skyline
File: app/src/main/cpp/skyline/common.h (C/C++)

Code before:
            class KProcess;
            class KThread;
        }
        class OS;
    }
    namespace audio {
        class Audio;
    }

    /**
     * @brief This struct is used to hold the state of a device
     */
    struct DeviceState {
        DeviceState(kernel::OS *os, std::shared_ptr<kernel::type::KProcess> &process, std::shared_ptr<JvmManager> jvmManager, std::shared_ptr<Settings> settings, std::shared_ptr<Logger> logger);

        kernel::OS *os; //!< This holds a reference to the OS class
        std::shared_ptr<kernel::type::KProcess> &process; //!< This holds a reference to the process object
        thread_local static std::shared_ptr<kernel::type::KThread> thread; //!< This holds a reference to the current thread object
        thread_local static ThreadContext *ctx; //!< This holds the context of the thread
        std::shared_ptr<NCE> nce; //!< This holds a reference to the NCE class
        std::shared_ptr<gpu::GPU> gpu; //!< This holds a reference to the GPU class
        std::shared_ptr<audio::Audio> audio; //!< This holds a reference to the Audio class
        std::shared_ptr<JvmManager> jvm; //!< This holds a reference to the JvmManager class
        std::shared_ptr<Settings> settings; //!< This holds a reference to the Settings class
        std::shared_ptr<Logger> logger; //!< This holds a reference to the Logger class
    };
}


Review comment (suggestion):
Consider if putting this in `OS` would be a better idea than having it here, if not then keeping it here is fine too.

Code after:
            class KProcess;
            class KThread;
        }
        class OS;
    }
    namespace audio {
        class Audio;
    }
    namespace loader {
        class Loader;
    }

    /**
     * @brief This struct is used to hold the state of a device
     */
    struct DeviceState {
        DeviceState(kernel::OS *os, std::shared_ptr<kernel::type::KProcess> &process, std::shared_ptr<JvmManager> jvmManager, std::shared_ptr<Settings> settings, std::shared_ptr<Logger> logger);

        kernel::OS *os; //!< This holds a reference to the OS class
        std::shared_ptr<kernel::type::KProcess> &process; //!< This holds a reference to the process object
        thread_local static std::shared_ptr<kernel::type::KThread> thread; //!< This holds a reference to the current thread object
        thread_local static ThreadContext *ctx; //!< This holds the context of the thread
        std::shared_ptr<NCE> nce; //!< This holds a reference to the NCE class
        std::shared_ptr<gpu::GPU> gpu; //!< This holds a reference to the GPU class
        std::shared_ptr<audio::Audio> audio; //!< This holds a reference to the Audio class
        std::shared_ptr<loader::Loader> loader; //!< This holds a reference to the Loader class
        std::shared_ptr<JvmManager> jvm; //!< This holds a reference to the JvmManager class
        std::shared_ptr<Settings> settings; //!< This holds a reference to the Settings class
        std::shared_ptr<Logger> logger; //!< This holds a reference to the Logger class
    };
}
Repository: skyline-emu/skyline
File: app/src/main/cpp/skyline/services/am/controller/ILibraryAppletCreator.cpp (C++)

Code before:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#include "ILibraryAppletCreator.h"

namespace skyline::service::am {
    ILibraryAppletCreator::ILibraryAppletCreator(const DeviceState &state, ServiceManager &manager) : BaseService(state, manager, Service::am_ILibraryAppletCreator, "am:ILibraryAppletCreator", {
    }) {}
}


Review comment (bug):
You might as well just read in an unsigned value as it being negative is just as invalid as it overflowing.

Code after:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#include <services/am/storage/IStorage.h>
#include <services/am/applet/ILibraryAppletAccessor.h>
#include "ILibraryAppletCreator.h"

namespace skyline::service::am {
    ILibraryAppletCreator::ILibraryAppletCreator(const DeviceState &state, ServiceManager &manager) : BaseService(state, manager, Service::am_ILibraryAppletCreator, "am:ILibraryAppletCreator", {
        {0x0, SFUNC(ILibraryAppletCreator::CreateLibraryApplet)},
        {0xa, SFUNC(ILibraryAppletCreator::CreateStorage)}
    }) {}

    void ILibraryAppletCreator::CreateLibraryApplet(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
        manager.RegisterService(SRVREG(ILibraryAppletAccessor), session, response);
    }

    void ILibraryAppletCreator::CreateStorage(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
        auto size = request.Pop<i64>();

        if (size < 0)
            throw exception("Cannot create an IStorage with a negative size");

        manager.RegisterService(std::make_shared<IStorage>(state, manager, size), session, response);
    }
}
Repository: skyline-emu/skyline
File: app/src/main/cpp/skyline/services/account/IProfile.h (C/C++)

Code before:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#pragma once

#include "IAccountServiceForApplication.h"
#include <services/base_service.h>

namespace skyline::service::account {

    /// UserData
    typedef struct {
        u32 unk_x0;                ///< Unknown.
        u32 iconID;                ///< Icon ID. 0 = Mii, the rest are character icon IDs.
        u8 iconBackgroundColorID;  ///< Profile icon background color ID
        u8 unk_x9[0x7];            ///< Unknown.
        u8 miiID[0x10];            ///< Some ID related to the Mii? All zeros when a character icon is used.
        u8 unk_x20[0x60];          ///< Usually zeros?
    } AccountUserData;

    /// ProfileBase
    typedef struct {
        UserId uid;        ///< \ref AccountUid
        u64 lastEditTimestamp; ///< POSIX UTC timestamp, for the last account edit.
        char nickname[0x20];   ///< UTF-8 Nickname.
    } AccountProfileBase;

    /**
    * @brief IProfile provides functions for reading user profile (https://switchbrew.org/wiki/Account_services#IProfile)
    */
    class IProfile : public BaseService {
      public:
        IProfile(const DeviceState &state, ServiceManager &manager, const UserId &userId);

      private:
        /**
         * @brief This returns AccountUserData (optional) and AccountProfileBase

Review comment (suggestion):
We don't use `typedef` for structures, just give them a type name directly `struct UserData`. In addition, considering this is only used once in the source file, you can just inline the structure and use it there directly. We'd also use a multi-line block comment describing the structure: 
```
/**
 * @brief ...
 */
```

Code after:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#pragma once

#include "IAccountServiceForApplication.h"
#include <services/base_service.h>

namespace skyline::service::account {

    /**
    * @brief IProfile provides functions for reading user profile (https://switchbrew.org/wiki/Account_services#IProfile)
    */
    class IProfile : public BaseService {
      public:
        IProfile(const DeviceState &state, ServiceManager &manager, const UserId &userId);

      private:
        UserId userId;

        /**
         * @brief This returns AccountUserData and AccountProfileBase objects that describe the user's information
         */
        void Get(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response);
    };
}
Repository: skyline-emu/skyline
File: app/src/main/cpp/skyline/services/account/IProfile.h (C/C++)

Code before:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#pragma once

#include "IAccountServiceForApplication.h"
#include <services/base_service.h>

namespace skyline::service::account {

    /// UserData
    typedef struct {
        u32 unk_x0;                ///< Unknown.
        u32 iconID;                ///< Icon ID. 0 = Mii, the rest are character icon IDs.
        u8 iconBackgroundColorID;  ///< Profile icon background color ID
        u8 unk_x9[0x7];            ///< Unknown.
        u8 miiID[0x10];            ///< Some ID related to the Mii? All zeros when a character icon is used.
        u8 unk_x20[0x60];          ///< Usually zeros?
    } AccountUserData;

    /// ProfileBase
    typedef struct {
        UserId uid;        ///< \ref AccountUid
        u64 lastEditTimestamp; ///< POSIX UTC timestamp, for the last account edit.
        char nickname[0x20];   ///< UTF-8 Nickname.
    } AccountProfileBase;

    /**
    * @brief IProfile provides functions for reading user profile (https://switchbrew.org/wiki/Account_services#IProfile)
    */
    class IProfile : public BaseService {
      public:
        IProfile(const DeviceState &state, ServiceManager &manager, const UserId &userId);

      private:
        /**
         * @brief This returns AccountUserData (optional) and AccountProfileBase
         */
        void Get(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response);

Review comment (suggestion):
The comments seem to be using `///<` instead of `//<`. We also tend to not use full-stops for inline comments, you can use a pair of brackets in this case `Icon ID (0 = Mii, the rest are character icon IDs)`.

Code after:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#pragma once

#include "IAccountServiceForApplication.h"
#include <services/base_service.h>

namespace skyline::service::account {

    /**
    * @brief IProfile provides functions for reading user profile (https://switchbrew.org/wiki/Account_services#IProfile)
    */
    class IProfile : public BaseService {
      public:
        IProfile(const DeviceState &state, ServiceManager &manager, const UserId &userId);

      private:
        UserId userId;

        /**
         * @brief This returns AccountUserData and AccountProfileBase objects that describe the user's information
         */
        void Get(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response);
    };
}
Repository: skyline-emu/skyline
File: app/src/main/cpp/skyline/services/account/IProfile.cpp (C++)

Code before:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#include <kernel/types/KProcess.h>
#include "IProfile.h"

namespace skyline::service::account {
    IProfile::IProfile(const DeviceState &state, ServiceManager &manager, const UserId &userId) : BaseService(state, manager, Service::account_IProfile, "account:IProfile", {
        {0x0, SFUNC(IProfile::Get)}
    }) {}

    void IProfile::Get(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
        AccountUserData userData{};
        AccountProfileBase profileBase{};

        std::string username = state.settings->GetString("username_value");
        size_t usernameSize = std::min(sizeof(profileBase), username.size());
        std::memcpy(profileBase.nickname, username.c_str(), usernameSize);
        profileBase.nickname[usernameSize] = '\0';

        state.process->WriteMemory(userData, request.outputBuf.at(0).address);
        response.Push(profileBase);
    }
}


Review comment (performance):
You can make this more efficient by using `state.process->GetPointer<AccountUserData>(request.outputBuf.at(0).address);` as the `userData` variable as it'll be 0-copy then. 

Code after:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#include <kernel/types/KProcess.h>
#include "IProfile.h"

namespace skyline::service::account {
    IProfile::IProfile(const DeviceState &state, ServiceManager &manager, const UserId &userId) : userId(userId), BaseService(state, manager, Service::account_IProfile, "account:IProfile", {
        {0x0, SFUNC(IProfile::Get)}
    }) {}

    void IProfile::Get(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
        struct AccountUserData {
            u32 _unk0_;
            u32 iconID;                        //!< Icon ID (0 = Mii, the rest are character icon IDs)
            u8 iconBackgroundColorID;          //!< Profile icon background color ID
            u8 _unk1_[0x7];
            std::array<u8, 0x10> miiID;        //!< Some ID related to the Mii? All zeros when a character icon is used
            u8 _unk2_[0x60];
        };

        struct {
            UserId uid;                        //!< The UID of the corresponding account
            u64 lastEditTimestamp;             //!< A POSIX UTC timestamp denoting the last account edit
            std::array<char, 0x20> nickname;   //!< UTF-8 Nickname
        } accountProfileBase = {
            .uid = userId
        };

        auto username = state.settings->GetString("username_value");
        size_t usernameSize = std::min(accountProfileBase.nickname.size() - 1, username.size());
        std::memcpy(accountProfileBase.nickname.data(), username.c_str(), usernameSize);

        auto userData = state.process->GetPointer<AccountUserData>(request.outputBuf.at(0).address);
        userData->iconBackgroundColorID = 0x1; // Color indexing starts at 0x1

        response.Push(accountProfileBase);
    }
}
Repository: skyline-emu/skyline
File: app/src/main/cpp/skyline/services/account/IProfile.cpp (C++)

Code before:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#include <kernel/types/KProcess.h>
#include "IProfile.h"

namespace skyline::service::account {
    IProfile::IProfile(const DeviceState &state, ServiceManager &manager, const UserId &userId) : BaseService(state, manager, Service::account_IProfile, "account:IProfile", {
        {0x0, SFUNC(IProfile::Get)}
    }) {}

    void IProfile::Get(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
        AccountUserData userData{};
        AccountProfileBase profileBase{};

        std::string username = state.settings->GetString("username_value");
        size_t usernameSize = std::min(sizeof(profileBase), username.size());
        std::memcpy(profileBase.nickname, username.c_str(), usernameSize);
        profileBase.nickname[usernameSize] = '\0';

        state.process->WriteMemory(userData, request.outputBuf.at(0).address);
        response.Push(profileBase);
    }
}


Review comment (suggestion):
You can only copy `usernameSize - 1` amount of characters, the last character will be implicitly 0 then, as you are 0-initializing the structure.

Code after:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#include <kernel/types/KProcess.h>
#include "IProfile.h"

namespace skyline::service::account {
    IProfile::IProfile(const DeviceState &state, ServiceManager &manager, const UserId &userId) : userId(userId), BaseService(state, manager, Service::account_IProfile, "account:IProfile", {
        {0x0, SFUNC(IProfile::Get)}
    }) {}

    void IProfile::Get(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response) {
        struct AccountUserData {
            u32 _unk0_;
            u32 iconID;                        //!< Icon ID (0 = Mii, the rest are character icon IDs)
            u8 iconBackgroundColorID;          //!< Profile icon background color ID
            u8 _unk1_[0x7];
            std::array<u8, 0x10> miiID;        //!< Some ID related to the Mii? All zeros when a character icon is used
            u8 _unk2_[0x60];
        };

        struct {
            UserId uid;                        //!< The UID of the corresponding account
            u64 lastEditTimestamp;             //!< A POSIX UTC timestamp denoting the last account edit
            std::array<char, 0x20> nickname;   //!< UTF-8 Nickname
        } accountProfileBase = {
            .uid = userId
        };

        auto username = state.settings->GetString("username_value");
        size_t usernameSize = std::min(accountProfileBase.nickname.size() - 1, username.size());
        std::memcpy(accountProfileBase.nickname.data(), username.c_str(), usernameSize);

        auto userData = state.process->GetPointer<AccountUserData>(request.outputBuf.at(0).address);
        userData->iconBackgroundColorID = 0x1; // Color indexing starts at 0x1

        response.Push(accountProfileBase);
    }
}
Repository: skyline-emu/skyline
File: app/src/main/cpp/skyline/services/nvdrv/devices/nvhost_ctrl.cpp (C++)

Code before:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#include <gpu.h>
#include <kernel/types/KProcess.h>
#include <services/nvdrv/driver.h>
#include "nvhost_ctrl.h"

namespace skyline::service::nvdrv::device {
    NvHostEvent::NvHostEvent(const DeviceState &state) : event(std::make_shared<type::KEvent>(state, false)) {}

    void NvHostEvent::Signal() {
        auto oldState{state};
        state = State::Signalling;

        // This is to ensure that the HOS event isn't signalled when the nvhost event is cancelled
        if (oldState == State::Waiting)
            event->Signal();

        state = State::Signalled;
    }

    void NvHostEvent::Cancel(const std::shared_ptr<gpu::GPU> &gpuState) {
        gpuState->syncpoints.at(fence.id).DeregisterWaiter(waiterId);
        Signal();
        event->ResetSignal();
    }


Review comment (bug):
Should we do source level attribution or move it to a more central location? I like the latter since I might forget to add attribution to a file I've used Ryujinx source code as reference in. You've also written the copyright clause incorrectly, there cannot be two of them. 

You would need to make it `Skyline Team, Ryujinx Team and Contributors` but `Ryujinx Team` would implicitly be counted in `Contributors` if we specified them as such in a centralized file which I'm leaning towards here.

Code after:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)
// Copyright © 2019-2020 Ryujinx Team and Contributors

#include <gpu.h>
#include <kernel/types/KProcess.h>
#include <services/nvdrv/driver.h>
#include "nvhost_ctrl.h"

namespace skyline::service::nvdrv::device {
    SyncpointEvent::SyncpointEvent(const DeviceState &state) : event(std::make_shared<type::KEvent>(state, false)) {}

    /**
     * @brief Metadata about a syncpoint event, used by QueryEvent and SyncpointEventWait
     */
    union SyncpointEventValue {
        u32 val;

        struct {
            u8 _pad0_ : 4;
            u32 syncpointIdAsync : 28;
        };

        struct {
            union {
                u8 eventSlotAsync;
                u16 eventSlotNonAsync;
            };
Repository: skyline-emu/skyline
File: app/src/main/cpp/skyline/common.h (C/C++)

Code before:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#pragma once

#include <map>
#include <unordered_map>
#include <span>
#include <list>
#include <vector>
#include <span>
#include <fstream>
#include <mutex>
#include <shared_mutex>
#include <functional>
#include <thread>
#include <string>
#include <cstdint>
#include <stdexcept>
#include <string>
#include <sstream>
#include <memory>
#include <compare>
#include <variant>
#include <sys/mman.h>
#include <fmt/format.h>
#include <frozen/unordered_map.h>
#include <frozen/string.h>
#include <jni.h>

#define FORCE_INLINE __attribute__((always_inline)) // NOLINT(cppcoreguidelines-macro-usage)

namespace fmt {
    /**
     * @brief A std::bitset formatter for {fmt}
     */
    template<size_t N>
    struct formatter<std::bitset<N>> : formatter<std::string> {
        template<typename FormatContext>
        constexpr auto format(const std::bitset<N> &s, FormatContext &ctx) {
            return formatter<std::string>::format(s.to_string(), ctx);
        }
    };
}

namespace skyline {
    using u128 = __uint128_t; //!< Unsigned 128-bit integer
    using u64 = __uint64_t; //!< Unsigned 64-bit integer
    using u32 = __uint32_t; //!< Unsigned 32-bit integer
    using u16 = __uint16_t; //!< Unsigned 16-bit integer

Review comment (suggestion):
You can remove the `#include <random>` from `common/uuid.cpp` which now has it redundantly included now that it's included here.

Code after:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#pragma once

#include <map>
#include <unordered_map>
#include <span>
#include <list>
#include <vector>
#include <span>
#include <fstream>
#include <mutex>
#include <shared_mutex>
#include <functional>
#include <thread>
#include <string>
#include <cstdint>
#include <stdexcept>
#include <string>
#include <sstream>
#include <memory>
#include <compare>
#include <variant>
#include <random>
#include <sys/mman.h>
#include <fmt/format.h>
#include <frozen/unordered_map.h>
#include <frozen/string.h>
#include <jni.h>

#define FORCE_INLINE __attribute__((always_inline)) // NOLINT(cppcoreguidelines-macro-usage)

namespace fmt {
    /**
     * @brief A std::bitset formatter for {fmt}
     */
    template<size_t N>
    struct formatter<std::bitset<N>> : formatter<std::string> {
        template<typename FormatContext>
        constexpr auto format(const std::bitset<N> &s, FormatContext &ctx) {
            return formatter<std::string>::format(s.to_string(), ctx);
        }
    };
}

namespace skyline {
    using u128 = __uint128_t; //!< Unsigned 128-bit integer
    using u64 = __uint64_t; //!< Unsigned 64-bit integer
    using u32 = __uint32_t; //!< Unsigned 32-bit integer
Repository: skyline-emu/skyline
File: app/src/main/cpp/skyline/services/bcat/IServiceCreator.h (C/C++)

Code before:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2020 Skyline Team and Contributors (https://github.com/skyline-emu/)

#pragma once

#include <services/serviceman.h>
#include "IBcatService.h"

namespace skyline::service::bcat {
    /**
     * @brief IApplicationProxyService is used to open an application proxy
     * @url https://switchbrew.org/wiki/Applet_Manager_services#appletOE
     */
    class IServiceCreator : public BaseService {
      public:
        IServiceCreator(const DeviceState &state, ServiceManager &manager);

        /**
         * @brief Takes an input u64 ProcessId, returns an #IBcatService
         * @url https://switchbrew.org/wiki/BCAT_services#bcat:a.2C_bcat:m.2C_bcat:u.2C_bcat:s
         */
        Result CreateBcatService(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response);

        SERVICE_DECL(
        SFUNC(0x0, IServiceCreator, CreateBcatService)
        )
    };
}


Review comment (suggestion):
I assume you copy-pasted the comment here from AM, you should correct the URL and make the brief something along the lines of `IServiceCreator is used to create per-process/per-title service instances for BCAT`.

Code after:
// SPDX-License-Identifier: MPL-2.0
// Copyright © 2022 Skyline Team and Contributors (https://github.com/skyline-emu/)

#pragma once

#include <services/serviceman.h>
#include "IBcatService.h"

namespace skyline::service::bcat {
    /**
     * @brief IServiceCreator is used to create per-process/per-title service instances for BCAT
     * @url https://switchbrew.org/wiki/BCAT_services#bcat:a.2C_bcat:m.2C_bcat:u.2C_bcat:s
     */
    class IServiceCreator : public BaseService {
      public:
        IServiceCreator(const DeviceState &state, ServiceManager &manager);

        /**
         * @brief Takes an input u64 ProcessId, returns an #IBcatService
         * @url https://switchbrew.org/wiki/BCAT_services#bcat:a.2C_bcat:m.2C_bcat:u.2C_bcat:s
         */
        Result CreateBcatService(type::KSession &session, ipc::IpcRequest &request, ipc::IpcResponse &response);

        SERVICE_DECL(
            SFUNC(0x0, IServiceCreator, CreateBcatService)
        )
    };
}