From b314b5c250644302c0a46fc89181c4d1b0b5f4c2 Mon Sep 17 00:00:00 2001 From: Nikola Kosturski Date: Thu, 25 Jul 2024 11:21:46 +0300 Subject: [PATCH] Address code review comments --- src/app/MessageDef/MessageDefHelper.cpp | 8 +-- src/app/server/Server.cpp | 8 +-- src/lib/core/CHIPConfig.h | 8 +-- src/lib/core/Global.h | 83 +++++++++++++------------ src/platform/Linux/CHIPPlatformConfig.h | 6 -- 5 files changed, 54 insertions(+), 59 deletions(-) diff --git a/src/app/MessageDef/MessageDefHelper.cpp b/src/app/MessageDef/MessageDefHelper.cpp index 6fe1bca0899ea8..112d99cf5ca2de 100644 --- a/src/app/MessageDef/MessageDefHelper.cpp +++ b/src/app/MessageDef/MessageDefHelper.cpp @@ -55,9 +55,9 @@ void PrettyPrintIMBlankLine() for (uint32_t i = 0; i < gPrettyPrintingDepthLevel; i++) { - if (gLineBuffer.size() > gCurLineBufferSize) + if (CHIP_CONFIG_LOG_MESSAGE_MAX_SIZE > gCurLineBufferSize) { - size_t sizeLeft = gLineBuffer.size() - gCurLineBufferSize; + size_t sizeLeft = CHIP_CONFIG_LOG_MESSAGE_MAX_SIZE - gCurLineBufferSize; size_t ret = (size_t) (snprintf(lineBuffer + gCurLineBufferSize, sizeLeft, "\t")); if (ret > 0) { @@ -77,9 +77,9 @@ void PrettyPrintIM(bool aIsNewLine, const char * aFmt, ...) PrettyPrintIMBlankLine(); } - if (gLineBuffer.size() > gCurLineBufferSize) + if (CHIP_CONFIG_LOG_MESSAGE_MAX_SIZE > gCurLineBufferSize) { - size_t sizeLeft = gLineBuffer.size() - gCurLineBufferSize; + size_t sizeLeft = CHIP_CONFIG_LOG_MESSAGE_MAX_SIZE - gCurLineBufferSize; size_t ret = (size_t) (vsnprintf(gLineBuffer.get() + gCurLineBufferSize, sizeLeft, aFmt, args)); if (ret > 0) { diff --git a/src/app/server/Server.cpp b/src/app/server/Server.cpp index 11f180f3eab406..4d3c08e43b67ca 100644 --- a/src/app/server/Server.cpp +++ b/src/app/server/Server.cpp @@ -260,12 +260,12 @@ CHIP_ERROR Server::Init(const ServerInitParams & initParams) { ::chip::app::LogStorageResources logStorageResources[] = { - { sDebugEventBuffer.get(), sDebugEventBuffer.size(), ::chip::app::PriorityLevel::Debug }, - { sInfoEventBuffer.get(), sInfoEventBuffer.size(), ::chip::app::PriorityLevel::Info }, - { sCritEventBuffer.get(), sCritEventBuffer.size(), ::chip::app::PriorityLevel::Critical } + { sDebugEventBuffer.get(), CHIP_DEVICE_CONFIG_EVENT_LOGGING_DEBUG_BUFFER_SIZE, ::chip::app::PriorityLevel::Debug }, + { sInfoEventBuffer.get(), CHIP_DEVICE_CONFIG_EVENT_LOGGING_INFO_BUFFER_SIZE, ::chip::app::PriorityLevel::Info }, + { sCritEventBuffer.get(), CHIP_DEVICE_CONFIG_EVENT_LOGGING_CRIT_BUFFER_SIZE, ::chip::app::PriorityLevel::Critical } }; - chip::app::EventManagement::GetInstance().Init(&mExchangeMgr, CHIP_NUM_EVENT_LOGGING_BUFFERS, &sLoggingBuffer[0], + chip::app::EventManagement::GetInstance().Init(&mExchangeMgr, CHIP_NUM_EVENT_LOGGING_BUFFERS, sLoggingBuffer.get(), &logStorageResources[0], &sGlobalEventIdCounter.get(), std::chrono::duration_cast(mInitTimestamp)); } diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h index c8c8fdc66ebf5e..2efa1716216a57 100644 --- a/src/lib/core/CHIPConfig.h +++ b/src/lib/core/CHIPConfig.h @@ -188,16 +188,16 @@ #endif // CHIP_CONFIG_GLOBALS_NO_DESTRUCT /** - * @def CHIP_CONFIG_DYNAMIC_GLOBALS + * @def CHIP_CONFIG_DYNAMICALLY_ALLOCATED_GLOBALS * * @brief * Whether to use dynamic allocation for chip::Global objects. * * The default is to use static allocation. */ -#ifndef CHIP_CONFIG_DYNAMIC_GLOBALS -#define CHIP_CONFIG_DYNAMIC_GLOBALS 0 -#endif // CHIP_CONFIG_DYNAMIC_GLOBALS +#ifndef CHIP_CONFIG_DYNAMICALLY_ALLOCATED_GLOBALS +#define CHIP_CONFIG_DYNAMICALLY_ALLOCATED_GLOBALS 0 +#endif // CHIP_CONFIG_DYNAMICALLY_ALLOCATED_GLOBALS /** * @def CHIP_CONFIG_SHA256_CONTEXT_SIZE diff --git a/src/lib/core/Global.h b/src/lib/core/Global.h index 3573998ade8d47..c17590c24e58fd 100644 --- a/src/lib/core/Global.h +++ b/src/lib/core/Global.h @@ -85,16 +85,6 @@ class Global /// NOT thread-safe, external synchronization is required. T & get() { return _get(); } T * operator->() { return &_get(); } - template > - U & operator[](std::size_t i) - { - return _get()[i]; - } - template > - constexpr std::size_t size() const - { - return N; - } // Globals are not copyable or movable Global(const Global &) = delete; @@ -107,59 +97,70 @@ class Global ~Global() = default; private: +#if CHIP_CONFIG_DYNAMICALLY_ALLOCATED_GLOBALS + std::remove_extent_t * mStorage = nullptr; + + T & _get() + { + mOnce.call(&create, this); + return *reinterpret_cast(mStorage); + } + + static void create(void * value) + { + auto self = static_cast *>(value); + self->mStorage = new T(); +#if !CHIP_CONFIG_GLOBALS_NO_DESTRUCT + CHIP_CXA_ATEXIT(&destroy, self->mStorage); +#endif // CHIP_CONFIG_GLOBALS_NO_DESTRUCT + } + + template , bool> = true> + static void destroy(void * value) + { + delete[] static_cast *>(value); + } + + template , bool> = true> + static void destroy(void * value) + { + delete static_cast(value); + } +#else // CHIP_CONFIG_DYNAMICALLY_ALLOCATED_GLOBALS // Zero-initialize everything. We should technically leave mStorage uninitialized, // but that can sometimes cause clang to be unable to constant-initialize the object. -#if CHIP_CONFIG_DYNAMIC_GLOBALS - unsigned char * mStorage = nullptr; -#else alignas(T) unsigned char mStorage[sizeof(T)] = {}; -#endif // CHIP_CONFIG_DYNAMIC_GLOBALS - OnceStrategy mOnce; T & _get() { -#if CHIP_CONFIG_DYNAMIC_GLOBALS - mOnce.call(&create, &mStorage); -#else mOnce.call(&create, mStorage); -#endif // CHIP_CONFIG_DYNAMIC_GLOBALS return *reinterpret_cast(mStorage); } + static void create(void * value) { -#if CHIP_CONFIG_DYNAMIC_GLOBALS - unsigned char ** storage = static_cast(value); - value = *storage = new unsigned char[sizeof(T)]; -#endif // CHIP_CONFIG_DYNAMIC_GLOBALS new (value) T(); #if !CHIP_CONFIG_GLOBALS_NO_DESTRUCT CHIP_CXA_ATEXIT(&destroy, value); #endif // CHIP_CONFIG_GLOBALS_NO_DESTRUCT } - template - static void _destroy(U (*value)[N]) - { - _destroy_array(*value, N); - } - template - static void _destroy_array(U * array, std::size_t N) - { - for (std::size_t i = 0; i < N; ++i) - array[i].~U(); - } - template - static void _destroy(U * value) + template , bool> = true> + static void destroy(void * value) { - value->~U(); + using V = std::remove_extent_t; + for (std::size_t i = 0; i != std::extent_v; ++i) + (static_cast(value) + i)->~V(); } + + template , bool> = true> static void destroy(void * value) { - _destroy(static_cast(value)); -#if CHIP_CONFIG_DYNAMIC_GLOBALS - delete[] static_cast(value); -#endif // CHIP_CONFIG_DYNAMIC_GLOBALS + static_cast(value)->~T(); } +#endif // CHIP_CONFIG_DYNAMICALLY_ALLOCATED_GLOBALS + + OnceStrategy mOnce; #else // CHIP_CONFIG_GLOBALS_LAZY_INIT public: diff --git a/src/platform/Linux/CHIPPlatformConfig.h b/src/platform/Linux/CHIPPlatformConfig.h index 7eb102b1a6c122..9e2832307f27b2 100644 --- a/src/platform/Linux/CHIPPlatformConfig.h +++ b/src/platform/Linux/CHIPPlatformConfig.h @@ -27,12 +27,6 @@ #define CHIP_CONFIG_ABORT() abort() -extern "C" int __cxa_atexit(void (*func)(void *), void * arg, void * d); -#define CHIP_CXA_ATEXIT(f, p) __cxa_atexit((f), (p), nullptr) - -#define CHIP_CONFIG_DYNAMIC_GLOBALS 1 -#define CHIP_CONFIG_GLOBALS_LAZY_INIT 1 - using CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE = const char *; #define CHIP_CONFIG_PERSISTED_STORAGE_MAX_KEY_LENGTH 16