From c3c5772e8acc2401c990df880e1fff3f4c369ad7 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sat, 15 Apr 2023 20:10:55 +0200 Subject: [PATCH 01/13] Add initial coding conventions markdown document --- documents/README.md | 1 + documents/development/README.md | 479 ++++++++++++++++++++++++++++++++ 2 files changed, 480 insertions(+) create mode 100644 documents/development/README.md diff --git a/documents/README.md b/documents/README.md index 85e2806b7..ea646f41d 100644 --- a/documents/README.md +++ b/documents/README.md @@ -88,3 +88,4 @@ bundles contains binaries depending on the stdlibc++ library. * [Apache Celix Patterns](patterns.md) * [Apache Celix CMake Commands](cmake_commands) * [Apache Celix Sub Projects](subprojects.md) +* [Apache Celix Coding Conventions Guide](development/README.md) diff --git a/documents/development/README.md b/documents/development/README.md new file mode 100644 index 000000000..6b12a1153 --- /dev/null +++ b/documents/development/README.md @@ -0,0 +1,479 @@ +--- +title: Apache Celix Coding Conventions +--- + + + +# Apache Celix Coding Conventions +Adhering to consistent and meaningful coding conventions is crucial for maintaining readable and maintainable code. +This document outlines the recommended coding conventions for Apache Celix development, including naming conventions, +formatting, comments, control structures, functions and error handling. + +Note that not all existing code adheres to these conventions. +New code should adhere to these conventions and when possible, existing code should be updated to adhere to these +conventions. + +## Naming Conventions + +### C/C++ Variables + +- Use `camelCase` for variable names. +- Use descriptive names for variables. +- Use `celix_` prefix for global variables. +- Asterisk `*` should be placed on the variable type name. + +### C Structures + +- Use `snake_case` for structure names. +- Add a typedef for the structure. +- Use a `_t` postfix for structure typedef. +- Use `celix_` prefix for structure names. + +### C Functions + +- Use `camelCase` for function names. +- Use descriptive names for functions. +- Use `celix_` prefix. +- Use a `__` camelCase infix for the object/module name. +- Use a postfix camelCase for the function name. +- Asterisk `*` should be placed on the variable type name. +- Use verb as function names when a function has a side effect. +- Use nouns or getter/setter as function names when a function does not have a side effect. +- Use getters/setters naming convention for functions which get/set a value. + +Examples: +- `long celix_bundleContext_installBundle(celix_bundle_context_t* ctx, const char* bundleUrl, bool autoStart)` +- `bool celix_utils_stringEquals(const char* a, const char* b)` +- `celix_status_t celix_utils_createDirectory(const char* path, bool failIfPresent, const char** errorOut)` + +### C Constants + +- Use `SNAKE_CASE` for constant names. +- Use a `CELIX_` prefix for constant names. +- Use `#define` for constants. + +### C Enums + +- Use `camelCase` for enum type names. +- Use a `celix_` prefix for enum type names. +- Use `SNAKE_CASE` for enum value names. +- Use a `CELIX_` prefix for enum value names. +- Add a typedef - with a `_e` postfix - for the enum + +Example: +```c +enum celix_bundleState { + CELIX_BUNDLE_STATE_UNKNOWN = 0x00000000, + CELIX_BUNDLE_STATE_UNINSTALLED = 0x00000001 +}; +typedef enum celix_bundleState celix_bundle_state_e; +``` + +### Macros + +- Use all caps `SNAKE_CASE` for macro names. +- Use a `CELIX_` prefix for macro names. + +### C files and directories + +- Use `snake_case` for file names. +- Name header files with a `.h` extension and source files with a `.c` extension. +- Organize files in directories according to their purpose. + - Public headers files in a `include`, `api` or `spi` directory. + - Private header files in a `private` and `src` directory. + - Source files in a `src` directory. +- Google test files should be placed in a `gtest` directory with its own `CMakeLists.txt` file and `src` directory. +- Use `celix_` prefix for header file names. +- Use a header guard. +- Use a C++ "extern C" block. + +### C Libraries + +- Target name should be `snake_case`. +- There should be `celix::` prefixed aliases for the library. +- C Shared libraries should configure an output name with a `celix_` prefix. + +### C Services + +- Service headers should be made available through a CMake INTERFACE header-only api/spi library (i.e. `celix::shell_api`) +- C service should be C struct, where the first member is the service handle (`void*`) and the rest of the members are + function pointers. +- The first argument of a service function should be the service handle (`void*`). +- If memory allocation is needed or another error can occur in a service function, ensure that the return value can + be used to check for errors. This can be done by: + - Returning a `celix_status_t` and if needed using an out parameter. + - Returning a NULL pointer if the function returns a pointer. + - Returning a boolean value, where `true` indicates success and `false` indicates failure. +- In the same header as the C service struct, there should be defines for the service name and version. +- The service macro name should be all caps `SNAKE_CASE` and should be prefixed with `CELIX_`. +- The service macro version should be all caps `SNAKE_CASE` and should be prefixed with `CELIX_`. +- The value of the service macro name should be the service struct (so without a `_t` postfix +- The value of the service macro version should be the version of the service. + +Example: +```c +//celix_foo.h + +#include "celix_errno.h" + +#define CELIX_FOO_SERVICE_NAME celix_foo_service +#define CELIX_FOO_SERVICE_VERSION 1.0.0 + +typedef struct celix_foo_service { + void* handle; + celix_status_t (*doFoo)(void* handle); +} celix_foo_service_t; +``` + +### C Bundles + +- Use `snake_case` for C bundle target names. +- Do _not_ use a `celix_` prefix for C bundle target names. +- Use `celix::` prefixed aliases for C bundle targets. +- Use `snake_case` for C bundle symbolic names. +- Configure at least SYMBOLIC_NAME, NAME, FILENAME, VERSION and GROUP for C bundle targets. +- Use `apache_celix_` prefix for C bundle symbolic names. +- Use `Apache Celix ` prefix for C bundle names. +- Use a `celix_` prefix for C bundle filenames. +- Use a group name starting with `celix/` for C bundle groups. + +Examples: +```cmake +add_celix_bundle(my_bundle + SOURCES src/my_bundle.c + SYMBOLIC_NAME "apache_celix_my_bundle" + NAME "Apache Celix My Bundle" + FILENAME "celix_my_bundle" + VERSION "1.0.0" + GROUP "celix/my_bundle_group" +) +add_library(celix::my_bundle ALIAS my_bundle) +``` + +### C++ Classes + +- Use `CamelCase` (starting with a capital) for class names. +- Use descriptive names for classes. +- Classes should be part of a `celix::` namespace or sub `celix::` namespace. + +### C++ Functions + +- Use `camelCase` for function names. +- If a function is part of a class, it should be part of a `celix::` namespace or sub `celix::` namespace. +- Asterisk `*` should be placed on the variable type name. +- Use verb as function names when a function has a side effect. +- Use nouns or getter/setter as function names when a function does not have a side effect. +- Use getters/setters naming convention for functions which get/set a value. + + +### C++ Constants + +- Use `SNAKE_CASE` for constants. +- Use constexpr for constants. +- Place constants in a `celix::` namespace or sub `celix::` namespace. + +example: +```c++ +namespace celix { + constexpr long FRAMEWORK_BUNDLE_ID = 0; + constexpr const char * const SERVICE_ID = "service.id"; +} +``` + +### C++ Enums + +- Use `CamelCase` (starting with a capital) for enum types names. +- For `enum class`, Use `SNAKE_CASE` for enum values without a celix/class prefix. Note that for values names + no prefix is required because enum class values are scoped. + +Example: +```c++ +namespace celix { + enum class ServiceRegistrationState { + REGISTERING, + REGISTERED, + UNREGISTERING, + UNREGISTERED + }; +} +``` + +### C++ files and directories + +- Use `CamelCase` (starting with a capital) for file names. +- Name header files with a `.h` extension and source files with a `.cc` extension. +- Place header files in a directory based on the namespace (e.g. `celix/Bundle.h`, `celix/dm/Component.h`). +- Organize files in directories according to their purpose. + - Public headers files in a `include`, `api` or `spi` directory. + - Private header files in a `private` and `src` directory. + - Source files in a `src` directory. +- For the Apache Celix framework and utils lib only header-only C++ files are allowed. +- Prefer header-only C++ files for the Apache Celix libraries +- Use a `#pragma once` header guard. + +### C++ Libraries + +- Target name should be `CamelCase` (starting with a capital). +- There should be `celix::` prefixed aliases for the library. +- C++ Libraries should support C++14. + - Exception are `celix::Promises` and `celix::PushStreams` which requires C++17. +- C++ Libraries should prefer to be header-only. +- C++ support for `celix::framework` and `celix::utils` must be header-only. +- Header-only C++ libraries do not need an export header and do not need to configure symbol visibility. +- C++ shared libraries (lib with C++ sources), should configure an output name with a `celix_` prefix. +- C++ shared libraries (lib with C++ sources), should use an export header and configure symbol visibility. + - See the C Libraries section for more information. + +### C++ Services + +- Use `CamelCase` (starting with a capital) for service names. +- Add a 'I' prefix to the service interface name. +- Place service classes in a `celix::` namespace or sub `celix::` namespace. +- Add a `static constexpr const char * const NAME` to the service class, for the service name. +- Add a `static constexpr const char * const SERVICE_VERSION` to the service class, for the service version. + +### C++ Bundles + +- Use `CamelCase` for C++ bundle target names. +- Do _not_ use a `Celix` prefix for C++ bundle target names. +- Use `celix::` prefixed aliases for C++ bundle targets. +- Use `CamelCase` for C++ bundle symbolic names. +- Configure at least SYMBOLIC_NAME, NAME, FILENAME, VERSION and GROUP for C++ bundle targets. +- Use `Apache_Celix_` prefix for C++ bundle symbolic names. +- Use `Apache Celix ` prefix for C++ bundle names. +- Use a `celix_` prefix for C++ bundle filenames. +- Use a group name starting with `celix/` for C++ bundle groups. + +Examples: +```cmake +add_celix_bundle(MyBundle + SOURCES src/MyBundle.cc + SYMBOLIC_NAME "Apache_Celix_MyBundle" + NAME "Apache Celix My Bundle" + FILENAME "celix_MyBundle" + VERSION "1.0.0" + GROUP "celix/MyBundleGroup" +) +add_library(celix::MyBundle ALIAS MyBundle) +``` + +### Unit Tests Naming + +- The test fixture should have a`TestSuite` postfix. +- The source file should be named after the test fixture name and use a `.cc` extension. +- Testcase names should use `CamelCase` (starting with a capital) and have a `Test` postfix. +- When using error injection (one of the `error_injector` libraries) a separate test suite should be used. + - A `ErrorInjectionTestSuite` postfix should be used for the test fixture. + - The error injection setup should be reset on the `TearDown` function or destructor of the test fixture. + +## Comments and Documentation + +- Use Doxygen for code documentation. +- Write comments that explain the purpose of the code, focusing on the "why" rather than the "how." +- Apply doxygen documentation to all public API's. +- Use the javadoc style for doxygen documentation. +- Use `@` instead of `\` for doxygen commands. +- Start with a `@brief` command and a short description. +- For `@param` commands also provide in, out, or in/out information. + +## Formatting and Indentation + +- Use spaces for indentation and use 4 spaces per indentation level. +- Keep line lengths under 80 characters, if possible, to enhance readability. +- Place opening braces on the same line as the control statement or function definition, + and closing braces on a new line aligned with the control statement or function definition. +- Use a single space before and after operators and around assignment statements. + + +## Control Structures + +- Always use braces ({ }) for control structures, even for single-statement blocks, to prevent errors. +- Use `if`, `else if`, and `else` statements to handle multiple conditions. +- Use `switch` statements for multiple conditions with a default case. +- Use `while` statements for loops that may not execute. +- Use `do`/`while` statements for loops that must execute at least once. +- Use `for` statements for loops with a known number of iterations. +- The use of `goto` is not allowed, except for error handling. +- For C, try to prevent deeply nested control structures and prefer early returns or `goto` statements. + - To prevent deeply nested control structures, the `CELIX_DO_IF` and `CELIX_GOTO_IF_ERR` macros can also be used. + +## Functions and Methods + +- Limit functions to a single responsibility or purpose, following the Single Responsibility Principle (SRP). +- Keep functions short and focused, aiming for a length of fewer than 50 lines. +- Ensure const correctness. + +## Error Handling and Logging + +- For C++, throw an exception when an error occurs. +- For C, if memory allocation is needed or another error can occur, ensure that a function returns a value + than can be used to check for errors. This can be done by: + - Returning a `celix_status_t` and if needed using an out parameter. + - Returning a NULL pointer if the function returns a pointer. + - Returning a boolean value, where `true` indicates success and `false` indicates failure. +- Use consistent error handling techniques, such as returning error codes or using designated error handling functions. +- Log errors, warnings, and other important events using the Apache Celix log helper functions or - for libraries - + the `celix_err` (TBD) functionality. +- Always check for errors and log them. +- Ensure error handling is correct, using test suite with error injection. + +## Error Injection + +- Use the Apache Celix error_injector libraries to inject errors in unit tests in a controlled way. +- Create a separate test suite for error injection tests. +- Reset error injection setup on the `TearDown` function or destructor of the test fixture. +- If an - internal or external - function is missing error injection support, add it to the error_injector library. + - Try to create small error injector libraries for specific functionality. + +## Unit Test Approach + +- Use the Google Test framework for unit tests. +- Use the Google Mock framework for mocking. +- Use the Apache Celix error_injector libraries to inject errors in unit tests in a controlled way. +- Test bundles by installing them in a programmatically created framework. +- Test bundles by using their provided services and used services. +- In most cases, libraries can be tested using a white box approach and bundles can be tested using a black box approach. + +## Supported C and C++ Standards + +- C libraries should support C99. (TBD or C11)) +- C++ libraries should support C++14. + - Exception are `celix::Promises` and `celix::PushStreams` which requires C++17. +- C++ support for `celix::framework` and `celix::utils` must be header-only. +- Unit test code can be written in C++17. + +## Library target properties + +For C and C++ shared libraries, the following target properties should be set: + - `VERSION` should be set to the library version. + - `SOVERSION` should be set to the library major version. + - `OUTPUT_NAME` should be set to the library name and should contain a `celix_` prefix. + +```cmake +add_library(my_lib SHARED + src/my_lib.c) +set_target_properties(my_lib + PROPERTIES + VERSION 1.0.0 + SOVERSION 1 + OUTPUT_NAME celix_my_lib) +``` + +For C and C++ static libraries, the following target properties should be set: + - `POSITION_INDEPENDENT_CODE` should be set to `ON` for static libraries. + - `OUTPUT_NAME` should be set to the library name and should contain a `celix_` prefix. + +```cmake +add_library(my_lib STATIC + src/my_lib.c) +set_target_properties(my_lib + PROPERTIES + POSITION_INDEPENDENT_CODE ON + OUTPUT_NAME celix_my_lib) +``` + +## Symbol Visibility + +- C libraries should configure symbol visibility. +- C++ header-only (INTERFACE) libraries should not configure symbol visibility. +- C++ shared libraries (lib with C++ sources), should configure symbol visibility. +- C and C++ Bundles should configure symbol visibility. + +### Configuring Symbol Visibility for C/C++ Libraries + +For Apache Celix shared libraries, symbol visibility should be configured using the CMake target +properties `C_VISIBILITY_PRESET`, `CXX_VISIBILITY_PRESET` and `VISIBILITY_INLINES_HIDDEN` and a generated export +header. + +The `C_VISIBILITY_PRESET` and `CXX_VISIBILITY_PRESET` target properties can be used to configure the default visibility +of symbols in C and C++ code. The `VISIBILITY_INLINES_HIDDEN` property can be used to configure the visibility of +inline functions. The `VISIBILITY_INLINES_HIDDEN` property is only supported for C++ code. + +The default visibility should be configured to hidden and symbols should be explictly exported using the export +marcos from a generated export header. The export header can be generated using the CMake function +`generate_export_header`. Every library should have its own export header. + +For shared libraries, this can be done using the following CMake code: + +TODO test if this works for C/C++ shared libraries +```cmake +add_library(my_lib SHARED + src/my_lib.c) +set_target_properties(my_lib PROPERTIES + C_VISIBILITY_PRESET hidden + #For C++ shared libraries also configure CXX_VISIBILITY_PRESET + #CXX_VISIBILITY_PRESET hidden + VISIBILITY_INLINES_HIDDEN ON + OUTPUT_NAME celix_my_lib) +target_include_directories(my_lib + PUBLIC + $ + $> + PRIVATE + src) + +#generate export header +generate_export_header(my_lib + BASE_NAME "CELIX_MY_LIB" + EXPORT_FILE_NAME "${CMAKE_BINARY_DIR}/celix/gen/includes/my_lib/celix_my_lib_export.h") +target_include_directories(my_bundle PRIVATE $) + +#install +install(TARGETS my_lib EXPORT celix LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} + INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/$) +install(DIRECTORY include/ + DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/$) +install(DIRECTORY ${CMAKE_BINARY_DIR}/celix/gen/includes/my_lib/ + DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/$) +``` + +For bundle, symbol visibility can be configured the same way as a shared library or the `HIDE_SYMBOLS` option in the +`add_celix_bundle` function can be used. + +The `HIDE_SYMBOLS` option in the `add_celix_bundle` will add a linker script that hides all symbols, expect for +the bundle activator symbols. Note the bundle activator symbols are needed to start and stop the bundle. + +```cmake +add_celix_bundle(my_bundle + SOURCES src/my_bundle.c + SYMBOLIC_NAME "apache_celix_my_bundle" + NAME "Apache Celix My Bundle" + FILENAME "celix_my_bundle" + VERSION "1.0.0" + GROUP "celix/my_bundle_group" + HIDE_SYMBOLS +) +add_library(celix::my_bundle ALIAS my_bundle) +``` + +## Benchmarking + +- When needed, use benchmarking to measure performance. +- Use the Google Benchmark framework for benchmarking. + +## Code Quality + +- New code should be reviewed through a pull request and no direct commits on the master branch are allowed. + - At least 1 reviewer should review the code. +- Hotfix pull request can be merged first and reviewed later, the rest is reviewed first and merged later. +- Unit tests should be written for all code. +- Code coverage should be measured and should be at least 95% for new code. +- For existing code, code coverage should be measured and should not decrease. +- Code should be checked for memory leaks using AddressSanitizer. +- Coverity scan are done on the master on a regular basis. Ideally new coverity issues should be fixed as soon as + possible. From d188dd40f04003a203ce9ed62033d5940e99de2c Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Fri, 5 May 2023 22:01:49 +0200 Subject: [PATCH 02/13] Update coding convention doc --- documents/development/README.md | 78 +++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 23 deletions(-) diff --git a/documents/development/README.md b/documents/development/README.md index 6b12a1153..4413f75da 100644 --- a/documents/development/README.md +++ b/documents/development/README.md @@ -284,7 +284,7 @@ add_library(celix::MyBundle ALIAS MyBundle) ## Comments and Documentation - Use Doxygen for code documentation. -- Write comments that explain the purpose of the code, focusing on the "why" rather than the "how." +- Write comments that explain the purpose of the code, focusing on the "why" rather than the "how". - Apply doxygen documentation to all public API's. - Use the javadoc style for doxygen documentation. - Use `@` instead of `\` for doxygen commands. @@ -294,11 +294,12 @@ add_library(celix::MyBundle ALIAS MyBundle) ## Formatting and Indentation - Use spaces for indentation and use 4 spaces per indentation level. -- Keep line lengths under 80 characters, if possible, to enhance readability. +- Keep line lengths under 120 characters, if possible, to enhance readability. - Place opening braces on the same line as the control statement or function definition, and closing braces on a new line aligned with the control statement or function definition. - Use a single space before and after operators and around assignment statements. - +- For new files apply clang-format using the project .clang-format file. + - Note that this can be done using a plugin for your IDE or by running `clang-format -i `. ## Control Structures @@ -317,7 +318,14 @@ add_library(celix::MyBundle ALIAS MyBundle) - Limit functions to a single responsibility or purpose, following the Single Responsibility Principle (SRP). - Keep functions short and focused, aiming for a length of fewer than 50 lines. - Ensure const correctness. - +- For C functions with a lot of different parameters, consider using an options struct. + - An options struct combined with a EMPTY_OPTIONS macro can be used to provide default values and a such + options struct can be updated backwards compatible. + - An options struct ensure that a lot of parameters can be configured, but also direct set on creation. +- For C++ functions with a lot of different parameters, consider using a builder pattern. + - A builder pattern can be updated backwards compatible. + - A builder pattern ensure that a lot of parameters can be configured, but also direct set on construction. + ## Error Handling and Logging - For C++, throw an exception when an error occurs. @@ -348,6 +356,30 @@ add_library(celix::MyBundle ALIAS MyBundle) - Test bundles by installing them in a programmatically created framework. - Test bundles by using their provided services and used services. - In most cases, libraries can be tested using a white box approach and bundles can be tested using a black box approach. +- For libraries that are tested with the Apache Celix error_injector libraries or require access to private/hidden + functions, a separate "code under test" static library should be created. + This library should not hide its symbols and should have a `_cut` postfix. + + +```cmake +set(MY_LIB_SOURCES ...) +set(MY_LIB_PUBLIC_LIBS ...) +set(MY_LIB_PRIVATE_LIBS ...) +add_library(my_lib SHARED ${MY_LIB_SOURCES}) +target_link_libraries(my_lib PUBLIC ${MY_LIB_PUBLIC_LIBS} PRIVATE ${MY_LIB_PRIVATE_LIBS}) +celix_target_hide_symbols(my_lib) +... + +if (ENABLE_TESTING) + add_library(my_lib_cut STATIC ${MY_LIB_SOURCES}) + target_link_libraries(my_lib PUBLIC ${MY_LIB_PUBLIC_LIBS} PRIVATE ${MY_LIB_PRIVATE_LIBS}) + target_include_directories(my_lib PUBLIC + ${CMAKE_CURRENT_LIST_DIR}/src + ${CMAKE_CURRENT_LIST_DIR}/include + ${CMAKE_BINARY_DIR}/celix/gen/includes/my_lib + ) +endif () +``` ## Supported C and C++ Standards @@ -389,10 +421,10 @@ set_target_properties(my_lib ## Symbol Visibility -- C libraries should configure symbol visibility. -- C++ header-only (INTERFACE) libraries should not configure symbol visibility. -- C++ shared libraries (lib with C++ sources), should configure symbol visibility. -- C and C++ Bundles should configure symbol visibility. +- Header-only (INTERFACE) libraries should not configure symbol visibility. +- Shared and static libraries should configure symbol visibility. + - Static library meant to be linked as PRIVATE should hide symbols. +- Bundles should configure symbol visibility (this is done by default). ### Configuring Symbol Visibility for C/C++ Libraries @@ -404,49 +436,49 @@ The `C_VISIBILITY_PRESET` and `CXX_VISIBILITY_PRESET` target properties can be u of symbols in C and C++ code. The `VISIBILITY_INLINES_HIDDEN` property can be used to configure the visibility of inline functions. The `VISIBILITY_INLINES_HIDDEN` property is only supported for C++ code. -The default visibility should be configured to hidden and symbols should be explictly exported using the export +The default visibility should be configured to hidden and symbols should be explicitly exported using the export marcos from a generated export header. The export header can be generated using the CMake function `generate_export_header`. Every library should have its own export header. For shared libraries, this can be done using the following CMake code: -TODO test if this works for C/C++ shared libraries ```cmake add_library(my_lib SHARED src/my_lib.c) set_target_properties(my_lib PROPERTIES C_VISIBILITY_PRESET hidden #For C++ shared libraries also configure CXX_VISIBILITY_PRESET - #CXX_VISIBILITY_PRESET hidden + CXX_VISIBILITY_PRESET hidden VISIBILITY_INLINES_HIDDEN ON OUTPUT_NAME celix_my_lib) target_include_directories(my_lib - PUBLIC - $ - $> - PRIVATE + PUBLIC + $ + $ + PRIVATE src) #generate export header generate_export_header(my_lib BASE_NAME "CELIX_MY_LIB" EXPORT_FILE_NAME "${CMAKE_BINARY_DIR}/celix/gen/includes/my_lib/celix_my_lib_export.h") -target_include_directories(my_bundle PRIVATE $) #install install(TARGETS my_lib EXPORT celix LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} - INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/$) + INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/celix_my_lib) install(DIRECTORY include/ - DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/$) + DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/celix_my_lib) install(DIRECTORY ${CMAKE_BINARY_DIR}/celix/gen/includes/my_lib/ - DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/$) + DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/celix_my_lib) ``` -For bundle, symbol visibility can be configured the same way as a shared library or the `HIDE_SYMBOLS` option in the -`add_celix_bundle` function can be used. +### Configuring Symbol Visibility for C/C++ Bundles + +For bundle, symbol visibility will default be configured to hidden. This can be default by providing +the `DO_NOT_CONFIGURE_SYMBOL_VISIBILITY` option to the CMake `add_celix_bundle` function. -The `HIDE_SYMBOLS` option in the `add_celix_bundle` will add a linker script that hides all symbols, expect for -the bundle activator symbols. Note the bundle activator symbols are needed to start and stop the bundle. +If symbol visibility is not configured in the `add_celix_bundle`, symbol visibility should be configured the same +way as a shared library. ```cmake add_celix_bundle(my_bundle From 7ec7c39f67495d5e89435eccec8ff8de137a22f1 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Fri, 5 May 2023 22:02:11 +0200 Subject: [PATCH 03/13] Add .clang-format file --- .clang-format | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 .clang-format diff --git a/.clang-format b/.clang-format new file mode 100644 index 000000000..6fde8dd54 --- /dev/null +++ b/.clang-format @@ -0,0 +1,25 @@ +Language: Cpp +BasedOnStyle: LLVM +AlignAfterOpenBracket: Align +AllowShortFunctionsOnASingleLine: Empty +AllowShortIfStatementsOnASingleLine: Never +AllowShortLoopsOnASingleLine: false +AlwaysBreakAfterReturnType: TopLevelDefinitions +BinPackParameters: false +BreakBeforeBraces: Attach +BreakConstructorInitializers: BeforeColon +ColumnLimit: 120 +ContinuationIndentWidth: 4 +Cpp11BracedListStyle: false +IndentCaseLabels: false +IndentWidth: 4 +KeepEmptyLinesAtTheStartOfBlocks: false +MaxEmptyLinesToKeep: 1 +NamespaceIndentation: None +PointerAlignment: Left +SortIncludes: false +SpaceAfterCStyleCast: true +SpaceBeforeParens: ControlStatements +SpacesInParentheses: false +TabWidth: 4 +UseTab: Never \ No newline at end of file From 65750a555bbe1085ab7b8f653b26ff6fc0cfa02f Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Fri, 5 May 2023 23:58:02 +0200 Subject: [PATCH 04/13] Update the naming convention documentation --- documents/development/README.md | 56 ++++++++++++++++----------------- 1 file changed, 27 insertions(+), 29 deletions(-) diff --git a/documents/development/README.md b/documents/development/README.md index 4413f75da..9ca13eac1 100644 --- a/documents/development/README.md +++ b/documents/development/README.md @@ -34,7 +34,7 @@ conventions. - Use `camelCase` for variable names. - Use descriptive names for variables. -- Use `celix_` prefix for global variables. +- Use `celix_` prefix or `celix::` (sub)namespace for global variables. - Asterisk `*` should be placed on the variable type name. ### C Structures @@ -46,11 +46,10 @@ conventions. ### C Functions -- Use `camelCase` for function names. - Use descriptive names for functions. -- Use `celix_` prefix. +- Use a `celix_` prefix. - Use a `__` camelCase infix for the object/module name. -- Use a postfix camelCase for the function name. +- Use a postfix `camelCase` for the function name. - Asterisk `*` should be placed on the variable type name. - Use verb as function names when a function has a side effect. - Use nouns or getter/setter as function names when a function does not have a side effect. @@ -69,7 +68,7 @@ Examples: ### C Enums -- Use `camelCase` for enum type names. +- Use `snake_case` for enum type names. - Use a `celix_` prefix for enum type names. - Use `SNAKE_CASE` for enum value names. - Use a `CELIX_` prefix for enum value names. @@ -77,11 +76,10 @@ Examples: Example: ```c -enum celix_bundleState { - CELIX_BUNDLE_STATE_UNKNOWN = 0x00000000, - CELIX_BUNDLE_STATE_UNINSTALLED = 0x00000001 -}; -typedef enum celix_bundleState celix_bundle_state_e; +typedef enum celix_hash_map_key_type { + CELIX_HASH_MAP_STRING_KEY, + CELIX_HASH_MAP_LONG_KEY +} celix_hash_map_key_type_e; ``` ### Macros @@ -100,44 +98,43 @@ typedef enum celix_bundleState celix_bundle_state_e; - Google test files should be placed in a `gtest` directory with its own `CMakeLists.txt` file and `src` directory. - Use `celix_` prefix for header file names. - Use a header guard. -- Use a C++ "extern C" block. +- Use a C++ "extern C" block in headers file to ensure C headers are usable in C++. ### C Libraries -- Target name should be `snake_case`. +- Target names should be `snake_case`. - There should be `celix::` prefixed aliases for the library. - C Shared libraries should configure an output name with a `celix_` prefix. ### C Services - Service headers should be made available through a CMake INTERFACE header-only api/spi library (i.e. `celix::shell_api`) -- C service should be C struct, where the first member is the service handle (`void*`) and the rest of the members are +- C service should be C struct, where the first member is the service handle (`void* handle;`) and the rest of the members are function pointers. -- The first argument of a service function should be the service handle (`void*`). +- The first argument of the service functions should be the service handle. - If memory allocation is needed or another error can occur in a service function, ensure that the return value can be used to check for errors. This can be done by: - Returning a `celix_status_t` and if needed using an out parameter. - - Returning a NULL pointer if the function returns a pointer. + - Returning a NULL pointer if the function returns a pointer type. - Returning a boolean value, where `true` indicates success and `false` indicates failure. - In the same header as the C service struct, there should be defines for the service name and version. -- The service macro name should be all caps `SNAKE_CASE` and should be prefixed with `CELIX_`. -- The service macro version should be all caps `SNAKE_CASE` and should be prefixed with `CELIX_`. -- The value of the service macro name should be the service struct (so without a `_t` postfix -- The value of the service macro version should be the version of the service. +- The service name macro should be all caps `SNAKE_CASE`, prefixed with `CELIX_` and postfixed with `_NAME`. +- The service version macro should be all caps `SNAKE_CASE`, prefixed with `CELIX_` and postfixed with `_VERSION`. +- The value of the service name macro should be the service struct (so without a `_t` postfix +- The value of the service version macro should be the version of the service. Example: ```c //celix_foo.h - #include "celix_errno.h" -#define CELIX_FOO_SERVICE_NAME celix_foo_service -#define CELIX_FOO_SERVICE_VERSION 1.0.0 +#define CELIX_FOO_NAME "celix_foo" +#define CELIX_FOO_VERSION 1.0.0 -typedef struct celix_foo_service { +typedef struct celix_foo { void* handle; - celix_status_t (*doFoo)(void* handle); -} celix_foo_service_t; + celix_status_t (*doFoo)(void* handle, char** outMsg); +} celix_foo_t; ``` ### C Bundles @@ -174,7 +171,7 @@ add_library(celix::my_bundle ALIAS my_bundle) ### C++ Functions - Use `camelCase` for function names. -- If a function is part of a class, it should be part of a `celix::` namespace or sub `celix::` namespace. +- If a function is not part of a class/struct, it should be part of a `celix::` namespace or sub `celix::` namespace. - Asterisk `*` should be placed on the variable type name. - Use verb as function names when a function has a side effect. - Use nouns or getter/setter as function names when a function does not have a side effect. @@ -198,8 +195,9 @@ namespace celix { ### C++ Enums - Use `CamelCase` (starting with a capital) for enum types names. -- For `enum class`, Use `SNAKE_CASE` for enum values without a celix/class prefix. Note that for values names - no prefix is required because enum class values are scoped. +- Use `enum class` instead of `enum`. +- Use `SNAKE_CASE` for enum values without a celix/class prefix. Note that for enum values no prefix is required + because enum class values are scoped. Example: ```c++ @@ -245,7 +243,7 @@ namespace celix { - Add a 'I' prefix to the service interface name. - Place service classes in a `celix::` namespace or sub `celix::` namespace. - Add a `static constexpr const char * const NAME` to the service class, for the service name. -- Add a `static constexpr const char * const SERVICE_VERSION` to the service class, for the service version. +- Add a `static constexpr const char * const VERSION` to the service class, for the service version. ### C++ Bundles From a565eff2e5564806f5921c9602bb6db81b5b798f Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sat, 6 May 2023 11:25:48 +0200 Subject: [PATCH 05/13] Update coding convention doc (#442) --- documents/development/README.md | 40 ++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/documents/development/README.md b/documents/development/README.md index 9ca13eac1..d16624f68 100644 --- a/documents/development/README.md +++ b/documents/development/README.md @@ -296,19 +296,22 @@ add_library(celix::MyBundle ALIAS MyBundle) - Place opening braces on the same line as the control statement or function definition, and closing braces on a new line aligned with the control statement or function definition. - Use a single space before and after operators and around assignment statements. +- Add a space after control keywords (`if`, `for`, `while`, etc.) that are followed by a parenthesis. +- Always use braces ({ }) for control structures, even for single-statement blocks, to prevent errors. +- Add a space after control keywords (`else`, `do`, etc) that are followed by a brace. +- Do not add a space after the function name and the opening parenthesis. - For new files apply clang-format using the project .clang-format file. - Note that this can be done using a plugin for your IDE or by running `clang-format -i `. ## Control Structures -- Always use braces ({ }) for control structures, even for single-statement blocks, to prevent errors. - Use `if`, `else if`, and `else` statements to handle multiple conditions. - Use `switch` statements for multiple conditions with a default case. - Use `while` statements for loops that may not execute. - Use `do`/`while` statements for loops that must execute at least once. - Use `for` statements for loops with a known number of iterations. - The use of `goto` is not allowed, except for error handling. -- For C, try to prevent deeply nested control structures and prefer early returns or `goto` statements. +- For C, try to prevent deeply nested control structures and prefer early returns or error handling `goto` statements. - To prevent deeply nested control structures, the `CELIX_DO_IF` and `CELIX_GOTO_IF_ERR` macros can also be used. ## Functions and Methods @@ -491,6 +494,33 @@ add_celix_bundle(my_bundle add_library(celix::my_bundle ALIAS my_bundle) ``` +## Branch naming + +- Prefix feature branches with `feature/`, hotfix branches with `hotfix/`, bugfix branches with `bugfix/` + and release branches with `release/`. +- If you are working on an issue, prefix the branch name with the issue number. E.g., `feature/1234-add-feature`. +- Hotfix branches are for urgent fixes that need to be applied as soon as possible. +- Use short and descriptive branch names. + +## Commit Messages + +- Utilize the imperative mood when writing commit messages (e.g., "Add feature" instead of "Adds feature" + or "Added feature"). This style aligns with git's auto-generated messages for merge commits or revert actions. +- Ensure that commit messages are descriptive and provide meaningful context. +- Keep the first line of the commit message concise, ideally under 50 characters. + This summary line serves as a quick overview of the change and should be easy to read in git logs. +- If more context is needed, separate the summary line from the body with a blank line. + The body can provide additional details, explanations, or reasoning behind the changes. + Aim to keep each line of the commit message body wrapped at around 72 characters for optimal readability. +- Use bullet points, numbered lists, or other formatting conventions when listing multiple changes or points in the + commit message body to improve readability. +- When applicable, reference related issues, bug reports, or pull requests in the commit message body to + provide additional context and establish connections between the commit and the larger project. + - If your commit fixes, closes, or resolves an issue, use one of these keywords followed by the issue number + (e.g., "fixes #42", "closes #42", or "resolves #42"). + - If you want to reference an issue without closing it, simply mention the issue number + (e.g., "related to #42" or "#42"). + ## Benchmarking - When needed, use benchmarking to measure performance. @@ -501,9 +531,9 @@ add_library(celix::my_bundle ALIAS my_bundle) - New code should be reviewed through a pull request and no direct commits on the master branch are allowed. - At least 1 reviewer should review the code. - Hotfix pull request can be merged first and reviewed later, the rest is reviewed first and merged later. -- Unit tests should be written for all code. -- Code coverage should be measured and should be at least 95% for new code. -- For existing code, code coverage should be measured and should not decrease. +- Unit tests should be written for all new code. +- Code coverage should be measured and strive for a minimum of 95% code coverage. +- For existing code, maintain or increase the code coverage. - Code should be checked for memory leaks using AddressSanitizer. - Coverity scan are done on the master on a regular basis. Ideally new coverity issues should be fixed as soon as possible. From 88ea1270a01085b44e7dc5463a8de1b8d95304a3 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sat, 6 May 2023 11:26:17 +0200 Subject: [PATCH 06/13] Update clang-format file (#508) --- .clang-format | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/.clang-format b/.clang-format index 6fde8dd54..f13cc0378 100644 --- a/.clang-format +++ b/.clang-format @@ -1,25 +1,27 @@ -Language: Cpp BasedOnStyle: LLVM -AlignAfterOpenBracket: Align -AllowShortFunctionsOnASingleLine: Empty -AllowShortIfStatementsOnASingleLine: Never -AllowShortLoopsOnASingleLine: false -AlwaysBreakAfterReturnType: TopLevelDefinitions +IndentWidth: 4 +AllowShortIfStatementsOnASingleLine: false +AlwaysBreakTemplateDeclarations: Yes +BinPackArguments: false BinPackParameters: false BreakBeforeBraces: Attach -BreakConstructorInitializers: BeforeColon ColumnLimit: 120 -ContinuationIndentWidth: 4 -Cpp11BracedListStyle: false -IndentCaseLabels: false -IndentWidth: 4 +ConstructorInitializerIndentWidth: 4 +Cpp11BracedListStyle: true +DerivePointerAlignment: false +IncludeBlocks: Regroup KeepEmptyLinesAtTheStartOfBlocks: false -MaxEmptyLinesToKeep: 1 NamespaceIndentation: None -PointerAlignment: Left +PointerAlignment: Right +ReflowComments: true SortIncludes: false -SpaceAfterCStyleCast: true +SpaceAfterCStyleCast: false +SpaceBeforeAssignmentOperators: true SpaceBeforeParens: ControlStatements +SpaceInEmptyParentheses: false +SpacesBeforeTrailingComments: 1 +SpacesInAngles: Never +SpacesInCStyleCastParentheses: false SpacesInParentheses: false -TabWidth: 4 -UseTab: Never \ No newline at end of file +SpacesInSquareBrackets: false +AlignEscapedNewlines: Right From b688dc060414ae2679833bb8b5d0a49f7ae5f1b0 Mon Sep 17 00:00:00 2001 From: PengZheng Date: Tue, 9 May 2023 21:33:07 +0800 Subject: [PATCH 07/13] Apply suggestions from code review --- documents/development/README.md | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/documents/development/README.md b/documents/development/README.md index d16624f68..f1b35efab 100644 --- a/documents/development/README.md +++ b/documents/development/README.md @@ -373,8 +373,8 @@ celix_target_hide_symbols(my_lib) if (ENABLE_TESTING) add_library(my_lib_cut STATIC ${MY_LIB_SOURCES}) - target_link_libraries(my_lib PUBLIC ${MY_LIB_PUBLIC_LIBS} PRIVATE ${MY_LIB_PRIVATE_LIBS}) - target_include_directories(my_lib PUBLIC + target_link_libraries(my_lib_cut PUBLIC ${MY_LIB_PUBLIC_LIBS} ${MY_LIB_PRIVATE_LIBS}) + target_include_directories(my_lib_cut PUBLIC ${CMAKE_CURRENT_LIST_DIR}/src ${CMAKE_CURRENT_LIST_DIR}/include ${CMAKE_BINARY_DIR}/celix/gen/includes/my_lib @@ -455,7 +455,6 @@ set_target_properties(my_lib PROPERTIES target_include_directories(my_lib PUBLIC $ - $ PRIVATE src) @@ -489,7 +488,6 @@ add_celix_bundle(my_bundle FILENAME "celix_my_bundle" VERSION "1.0.0" GROUP "celix/my_bundle_group" - HIDE_SYMBOLS ) add_library(celix::my_bundle ALIAS my_bundle) ``` From ef45f41c281f96d16f8a2bb7f4329aaad3cd008d Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Tue, 16 May 2023 12:20:07 +0200 Subject: [PATCH 08/13] Applied some of the review comments for PR #544 --- .clang-format | 3 +- CMakeLists.txt | 2 +- documents/development/README.md | 102 +++++++++++++++++++++++++++---- libs/utils/include/celix_errno.h | 7 ++- 4 files changed, 98 insertions(+), 16 deletions(-) diff --git a/.clang-format b/.clang-format index f13cc0378..784622d9c 100644 --- a/.clang-format +++ b/.clang-format @@ -1,4 +1,5 @@ BasedOnStyle: LLVM +Language: Cpp IndentWidth: 4 AllowShortIfStatementsOnASingleLine: false AlwaysBreakTemplateDeclarations: Yes @@ -12,7 +13,7 @@ DerivePointerAlignment: false IncludeBlocks: Regroup KeepEmptyLinesAtTheStartOfBlocks: false NamespaceIndentation: None -PointerAlignment: Right +PointerAlignment: Left ReflowComments: true SortIncludes: false SpaceAfterCStyleCast: false diff --git a/CMakeLists.txt b/CMakeLists.txt index dbc06c44e..d3d504e1a 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -70,7 +70,7 @@ if (ENABLE_TESTING) endif () # Set C specific flags -set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=gnu99 -fPIC ${CMAKE_C_FLAGS}") +set(CMAKE_C_FLAGS "-D_GNU_SOURCE -std=c99 -fPIC ${CMAKE_C_FLAGS}") set(CMAKE_C_FLAGS "-Wall -Werror -Wformat -Wno-error=deprecated-declarations ${CMAKE_C_FLAGS}") # Set C++ specific flags diff --git a/documents/development/README.md b/documents/development/README.md index f1b35efab..550e58e73 100644 --- a/documents/development/README.md +++ b/documents/development/README.md @@ -35,7 +35,7 @@ conventions. - Use `camelCase` for variable names. - Use descriptive names for variables. - Use `celix_` prefix or `celix::` (sub)namespace for global variables. -- Asterisk `*` should be placed on the variable type name. +- Asterisks `*` and ampersands `&` should be placed on the variable type name. ### C Structures @@ -43,6 +43,8 @@ conventions. - Add a typedef for the structure. - Use a `_t` postfix for structure typedef. - Use `celix_` prefix for structure names. +- For C objects, use typedef of an opaque struct. E.g. `typedef struct celix_ celix__t;` + - This way the implementation details can be hidden from the user. ### C Functions @@ -50,10 +52,18 @@ conventions. - Use a `celix_` prefix. - Use a `__` camelCase infix for the object/module name. - Use a postfix `camelCase` for the function name. -- Asterisk `*` should be placed on the variable type name. +- Asterisks `*` should be placed on the variable type name. - Use verb as function names when a function has a side effect. - Use nouns or getter/setter as function names when a function does not have a side effect. - Use getters/setters naming convention for functions which get/set a value. +- For C objects: + - Use a (opaque) object pointer as the first argument of the function. + - Ensure that object can be created using a `celix__create` function and destroyed using + a `celix__destroy` function. + - The `celix__create` function should return a pointer to the object. + - The `celix__destroy` function should return a `void` and should be able to handle a NULL pointer. + - By being able to handle a NULL pointer, the `celix__destroy` function can be more easily used in + error handling code. Examples: - `long celix_bundleContext_installBundle(celix_bundle_context_t* ctx, const char* bundleUrl, bool autoStart)` @@ -162,6 +172,13 @@ add_celix_bundle(my_bundle add_library(celix::my_bundle ALIAS my_bundle) ``` +### C++ Namespaces + +- Use `snake_case` for namespace names. +- All namespaces should be part of the `celix` namespace. +- Aim for a max of 3 levels of namespaces. +- Use a namespace ending with `impl` for implementation details. + ### C++ Classes - Use `CamelCase` (starting with a capital) for class names. @@ -172,7 +189,7 @@ add_library(celix::my_bundle ALIAS my_bundle) - Use `camelCase` for function names. - If a function is not part of a class/struct, it should be part of a `celix::` namespace or sub `celix::` namespace. -- Asterisk `*` should be placed on the variable type name. +- Asterisks `*` and ampersands `&` should be placed on the variable type name. - Use verb as function names when a function has a side effect. - Use nouns or getter/setter as function names when a function does not have a side effect. - Use getters/setters naming convention for functions which get/set a value. @@ -188,7 +205,7 @@ example: ```c++ namespace celix { constexpr long FRAMEWORK_BUNDLE_ID = 0; - constexpr const char * const SERVICE_ID = "service.id"; + constexpr const char* const SERVICE_ID = "service.id"; } ``` @@ -242,8 +259,8 @@ namespace celix { - Use `CamelCase` (starting with a capital) for service names. - Add a 'I' prefix to the service interface name. - Place service classes in a `celix::` namespace or sub `celix::` namespace. -- Add a `static constexpr const char * const NAME` to the service class, for the service name. -- Add a `static constexpr const char * const VERSION` to the service class, for the service version. +- Add a `static constexpr const char* const NAME` to the service class, for the service name. +- Add a `static constexpr const char* const VERSION` to the service class, for the service version. ### C++ Bundles @@ -281,14 +298,18 @@ add_library(celix::MyBundle ALIAS MyBundle) ## Comments and Documentation -- Use Doxygen for code documentation. +- Use Doxygen documentation, except for inline comments. - Write comments that explain the purpose of the code, focusing on the "why" rather than the "how". - Apply doxygen documentation to all public API's. - Use the javadoc style for doxygen documentation. - Use `@` instead of `\` for doxygen commands. - Start with a `@brief` command and a short description. - For `@param` commands also provide in, out, or in/out information. +- For `@return` commands also provide a description of the return value. +- If a function can return multiple error codes, use a errors section (`@section errors_section Errors`) to document the + possible errors. Use `man 2 write` as an example for a good errors section. + ## Formatting and Indentation - Use spaces for indentation and use 4 spaces per indentation level. @@ -310,9 +331,10 @@ add_library(celix::MyBundle ALIAS MyBundle) - Use `while` statements for loops that may not execute. - Use `do`/`while` statements for loops that must execute at least once. - Use `for` statements for loops with a known number of iterations. -- The use of `goto` is not allowed, except for error handling. +- The use of `goto` is not allowed, except for error handling in C (for C++ use RAII). - For C, try to prevent deeply nested control structures and prefer early returns or error handling `goto` statements. - - To prevent deeply nested control structures, the `CELIX_DO_IF` and `CELIX_GOTO_IF_ERR` macros can also be used. + - To prevent deeply nested control structures, the `CELIX_DO_IF`, `CELIX_GOTO_IF_NULL` and `CELIX_GOTO_IF_ERR` + macros can also be used. ## Functions and Methods @@ -329,7 +351,7 @@ add_library(celix::MyBundle ALIAS MyBundle) ## Error Handling and Logging -- For C++, throw an exception when an error occurs. +- For C++, throw an exception when an error occurs and use RAII to ensure that resources are freed. - For C, if memory allocation is needed or another error can occur, ensure that a function returns a value than can be used to check for errors. This can be done by: - Returning a `celix_status_t` and if needed using an out parameter. @@ -337,14 +359,68 @@ add_library(celix::MyBundle ALIAS MyBundle) - Returning a boolean value, where `true` indicates success and `false` indicates failure. - Use consistent error handling techniques, such as returning error codes or using designated error handling functions. - Log errors, warnings, and other important events using the Apache Celix log helper functions or - for libraries - - the `celix_err` (TBD) functionality. + the `celix_err` functionality. - Always check for errors and log them. +- Error handling should free resources in the reverse order of their allocation/creation. - Ensure error handling is correct, using test suite with error injection. +For log levels use the following guidelines: +- trace: Use this level for very detailed that you would only want to have while diagnosing problems. +- debug: This level should be used for information that might be helpful in diagnosing problems or understanding + what's going on, but is too verbose to be enabled by default. +- info: Use this level for general operational messages that aren't tied to any specific problem or error condition. + They provide insight into the normal behavior of the system. Examples include startup/shutdown messages, + configuration assumptions, etc. +- warning: Use this level to report an issue from which the system can recover, but which might indicate a potential + problem. +- error: This level should be used to report issues that need immediate attention and might prevent the system from + functioning correctly. These are problems that are unexpected and affect functionality, but not so severe that the + process needs to stop. Examples include runtime errors, inability to connect to a service, etc. +- fatal: Use this level to report severe errors that prevent the program from continuing to run. + After logging a fatal error, the program will typically terminate. + +Example of error handling and logging: +```c +celix_foo_t* celix_foo_create(celix_log_helper_t* logHelper) { + celix_foo_t* foo = calloc(1, sizeof(*foo)); + if (!foo) { + goto create_enomem_err; + } + + CELIX_GOTO_IF_ERR(create_mutex_err, celixThreadMutex_create(&foo->mutex, NULL)); + + foo->list = celix_arrayList_create(); + foo->map = celix_longHashMap_create(); + if (!foo->list || !foo->map) { + goto create_enomem_err; + } + + return foo; +create_mutex_err: + celix_logHelper_log(logHelper, CELIX_LOG_LEVEL_ERROR, "Error creating mutex"); + free(foo); //mutex not created, do not use celix_foo_destroy to prevent mutex destroy + return NULL; +create_enomem_err: + celix_logHelper_log(logHelper, CELIX_LOG_LEVEL_ERROR, "Error creating foo, out of memory"); + celix_foo_destroy(foo); //note celix_foo_destroy can handle NULL + return NULL; +} + +void celix_foo_destroy(celix_foo_t* foo) { + if (foo != NULL) { + //note reverse order of creation + celixThreadMutex_destroy(&foo->mutex); + celix_arrayList_destroy(foo->list); + celix_longHashMap_destroy(foo->map); + free(foo); + } +} +``` + ## Error Injection - Use the Apache Celix error_injector libraries to inject errors in unit tests in a controlled way. -- Create a separate test suite for error injection tests. +- Create a separate test suite for error injection tests and place them under a `LINKER_WRAP_SUPPORTED` cmake condition. - Reset error injection setup on the `TearDown` function or destructor of the test fixture. - If an - internal or external - function is missing error injection support, add it to the error_injector library. - Try to create small error injector libraries for specific functionality. @@ -384,7 +460,7 @@ endif () ## Supported C and C++ Standards -- C libraries should support C99. (TBD or C11)) +- C libraries should support C99. - C++ libraries should support C++14. - Exception are `celix::Promises` and `celix::PushStreams` which requires C++17. - C++ support for `celix::framework` and `celix::utils` must be header-only. diff --git a/libs/utils/include/celix_errno.h b/libs/utils/include/celix_errno.h index 9e5e92b2e..d084575fb 100644 --- a/libs/utils/include/celix_errno.h +++ b/libs/utils/include/celix_errno.h @@ -56,7 +56,12 @@ extern "C" { * Helper macro which check the current status and executes a goto the provided label if the * status is not CELIX_SUCCESS (0) */ -#define CELIX_GOTO_IF_ERR(status, label) if ((status) != CELIX_SUCCESS) { goto label; } +#define CELIX_GOTO_IF_ERR(label, status) \ + do { \ + if ((status) != CELIX_SUCCESS) { \ + goto label; \ + } \ + } while (0) /*! * \defgroup celix_errno Error Codes From 8ed95f2c72a6b9bce3bf4cf2640e1118d3d3a57f Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Tue, 16 May 2023 22:31:47 +0200 Subject: [PATCH 09/13] Update dev guidelines and clang-format Also updates some source files to test and show clang-format header grouping result. --- .clang-format | 20 ++++++++++++++++-- .../admin/src/RemoteServiceAdmin.cc | 1 + documents/development/README.md | 14 +++++++------ libs/framework/src/bundle_archive.c | 21 +++++++++---------- libs/utils/include/celix/Filter.h | 3 ++- libs/utils/include/celix_errno.h | 2 +- 6 files changed, 40 insertions(+), 21 deletions(-) diff --git a/.clang-format b/.clang-format index 784622d9c..11a583ca9 100644 --- a/.clang-format +++ b/.clang-format @@ -10,12 +10,10 @@ ColumnLimit: 120 ConstructorInitializerIndentWidth: 4 Cpp11BracedListStyle: true DerivePointerAlignment: false -IncludeBlocks: Regroup KeepEmptyLinesAtTheStartOfBlocks: false NamespaceIndentation: None PointerAlignment: Left ReflowComments: true -SortIncludes: false SpaceAfterCStyleCast: false SpaceBeforeAssignmentOperators: true SpaceBeforeParens: ControlStatements @@ -26,3 +24,21 @@ SpacesInCStyleCastParentheses: false SpacesInParentheses: false SpacesInSquareBrackets: false AlignEscapedNewlines: Right +SortIncludes: true +IncludeBlocks: Regroup +IncludeCategories: +#gtest headers + - Regex: '^((<|")gtest/)' + Priority: 1 +#stdlib C++ headers + - Regex: '^<([^.])*>' + Priority: 2 +#external headers + - Regex: '^<([^.])*\.h>' + Priority: 3 +#celix C API + - Regex: '^(<|")celix_.*' + Priority: 4 +#celix C++ API + - Regex: '^(<|")celix/.*' + Priority: 5 diff --git a/bundles/cxx_remote_services/admin/src/RemoteServiceAdmin.cc b/bundles/cxx_remote_services/admin/src/RemoteServiceAdmin.cc index 116820cd5..962eea716 100644 --- a/bundles/cxx_remote_services/admin/src/RemoteServiceAdmin.cc +++ b/bundles/cxx_remote_services/admin/src/RemoteServiceAdmin.cc @@ -18,6 +18,7 @@ */ #include "RemoteServiceAdmin.h" + #include "celix/BundleContext.h" #include "celix/rsa/RemoteConstants.h" diff --git a/documents/development/README.md b/documents/development/README.md index 550e58e73..7de1cc8b4 100644 --- a/documents/development/README.md +++ b/documents/development/README.md @@ -55,7 +55,9 @@ conventions. - Asterisks `*` should be placed on the variable type name. - Use verb as function names when a function has a side effect. - Use nouns or getter/setter as function names when a function does not have a side effect. -- Use getters/setters naming convention for functions which get/set a value. +- Use getters/setters naming convention for functions which get/set a value: + - `celix__is` and `celix__set` for boolean values + - `celix__get` and `celix__set` for other values - For C objects: - Use a (opaque) object pointer as the first argument of the function. - Ensure that object can be created using a `celix__create` function and destroyed using @@ -237,8 +239,6 @@ namespace celix { - Public headers files in a `include`, `api` or `spi` directory. - Private header files in a `private` and `src` directory. - Source files in a `src` directory. -- For the Apache Celix framework and utils lib only header-only C++ files are allowed. -- Prefer header-only C++ files for the Apache Celix libraries - Use a `#pragma once` header guard. ### C++ Libraries @@ -247,8 +247,10 @@ namespace celix { - There should be `celix::` prefixed aliases for the library. - C++ Libraries should support C++14. - Exception are `celix::Promises` and `celix::PushStreams` which requires C++17. -- C++ Libraries should prefer to be header-only. -- C++ support for `celix::framework` and `celix::utils` must be header-only. +- The Apache Celix framework library (`Celix::framework`) and the Apache Celix utils library (`Celix::utils`) can only + use header-only C++ files. This ensure that the framework and utils library can be used in C only projects and do + not introduce a C++ ABI. +- For other libraries, header-only C++ libraries are preferred but not required. - Header-only C++ libraries do not need an export header and do not need to configure symbol visibility. - C++ shared libraries (lib with C++ sources), should configure an output name with a `celix_` prefix. - C++ shared libraries (lib with C++ sources), should use an export header and configure symbol visibility. @@ -434,7 +436,7 @@ void celix_foo_destroy(celix_foo_t* foo) { - Test bundles by using their provided services and used services. - In most cases, libraries can be tested using a white box approach and bundles can be tested using a black box approach. - For libraries that are tested with the Apache Celix error_injector libraries or require access to private/hidden - functions, a separate "code under test" static library should be created. + functions (white-box testing), a separate "code under test" static library should be created. This library should not hide its symbols and should have a `_cut` postfix. diff --git a/libs/framework/src/bundle_archive.c b/libs/framework/src/bundle_archive.c index b06300b86..1cffa72ee 100644 --- a/libs/framework/src/bundle_archive.c +++ b/libs/framework/src/bundle_archive.c @@ -17,27 +17,26 @@ * under the License. */ -#include "bundle_archive_private.h" +#include "bundle_archive.h" -#include +#include +#include #include #include -#include -#include +#include #include -#include +#include #include -#include -#include #include "celix_constants.h" -#include "celix_utils_api.h" -#include "linked_list_iterator.h" -#include "framework_private.h" #include "celix_file_utils.h" -#include "bundle_revision_private.h" #include "celix_framework_utils_private.h" +#include "celix_utils_api.h" +#include "bundle_archive_private.h" +#include "bundle_revision_private.h" +#include "framework_private.h" +#include "linked_list_iterator.h" celix_status_t celix_bundleArchive_getLastModifiedInternal(bundle_archive_pt archive, struct timespec *lastModified); diff --git a/libs/utils/include/celix/Filter.h b/libs/utils/include/celix/Filter.h index f0a02d9f0..c59283043 100644 --- a/libs/utils/include/celix/Filter.h +++ b/libs/utils/include/celix/Filter.h @@ -19,10 +19,11 @@ #pragma once -#include #include +#include #include "celix_filter.h" + #include "celix/Properties.h" namespace celix { diff --git a/libs/utils/include/celix_errno.h b/libs/utils/include/celix_errno.h index 70dc529ad..82870e130 100644 --- a/libs/utils/include/celix_errno.h +++ b/libs/utils/include/celix_errno.h @@ -60,7 +60,7 @@ extern "C" { * Helper macro which check the current status and executes a goto the provided label if the * status is not CELIX_SUCCESS (0) */ -#define CELIX_GOTO_IF_ERR(label, status) \ +#define CELIX_GOTO_IF_ERR(status, label) \ do { \ if ((status) != CELIX_SUCCESS) { \ goto label; \ From ec718724b9060834d80d2e1ee8d8b9afe8338814 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Mon, 22 May 2023 22:44:31 +0200 Subject: [PATCH 10/13] Remove redundant clang-format options --- .clang-format | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/.clang-format b/.clang-format index 11a583ca9..1e86f6120 100644 --- a/.clang-format +++ b/.clang-format @@ -5,25 +5,11 @@ AllowShortIfStatementsOnASingleLine: false AlwaysBreakTemplateDeclarations: Yes BinPackArguments: false BinPackParameters: false -BreakBeforeBraces: Attach ColumnLimit: 120 -ConstructorInitializerIndentWidth: 4 -Cpp11BracedListStyle: true DerivePointerAlignment: false KeepEmptyLinesAtTheStartOfBlocks: false -NamespaceIndentation: None PointerAlignment: Left -ReflowComments: true -SpaceAfterCStyleCast: false -SpaceBeforeAssignmentOperators: true -SpaceBeforeParens: ControlStatements -SpaceInEmptyParentheses: false SpacesBeforeTrailingComments: 1 -SpacesInAngles: Never -SpacesInCStyleCastParentheses: false -SpacesInParentheses: false -SpacesInSquareBrackets: false -AlignEscapedNewlines: Right SortIncludes: true IncludeBlocks: Regroup IncludeCategories: From e32f4d5f9cd31a55b5c8cf427b2c42a139927a26 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sun, 4 Jun 2023 19:17:07 +0200 Subject: [PATCH 11/13] Use std::int8_t as enum class base --- libs/framework/include/celix/Bundle.h | 2 +- libs/framework/include/celix/ServiceRegistration.h | 2 +- libs/framework/include/celix/Trackers.h | 2 +- libs/framework/include/celix/dm/Component.h | 2 +- libs/pushstreams/api/celix/PushEvent.h | 2 +- libs/pushstreams/api/celix/PushStream.h | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/libs/framework/include/celix/Bundle.h b/libs/framework/include/celix/Bundle.h index 28fe055b1..5cc5e73f3 100644 --- a/libs/framework/include/celix/Bundle.h +++ b/libs/framework/include/celix/Bundle.h @@ -26,7 +26,7 @@ namespace celix { - enum class BundleState { + enum class BundleState : std::uint8_t { UNKNOWN, UNINSTALLED, INSTALLED, diff --git a/libs/framework/include/celix/ServiceRegistration.h b/libs/framework/include/celix/ServiceRegistration.h index 6f7b4f0a8..a89610e55 100644 --- a/libs/framework/include/celix/ServiceRegistration.h +++ b/libs/framework/include/celix/ServiceRegistration.h @@ -33,7 +33,7 @@ namespace celix { - enum class ServiceRegistrationState { + enum class ServiceRegistrationState : std::uint8_t { REGISTERING, REGISTERED, UNREGISTERING, diff --git a/libs/framework/include/celix/Trackers.h b/libs/framework/include/celix/Trackers.h index 4314b30df..78d7a0011 100644 --- a/libs/framework/include/celix/Trackers.h +++ b/libs/framework/include/celix/Trackers.h @@ -45,7 +45,7 @@ namespace celix { /** * @brief The tracker state. */ - enum class TrackerState { + enum class TrackerState : std::uint8_t { OPENING, OPEN, CLOSING, diff --git a/libs/framework/include/celix/dm/Component.h b/libs/framework/include/celix/dm/Component.h index a1a429dd9..cf13f39b3 100644 --- a/libs/framework/include/celix/dm/Component.h +++ b/libs/framework/include/celix/dm/Component.h @@ -40,7 +40,7 @@ namespace celix { namespace dm { - enum class ComponentState { + enum class ComponentState : std::uint8_t { INACTIVE = 1, WAITING_FOR_REQUIRED = 2, INITIALIZING = 3, diff --git a/libs/pushstreams/api/celix/PushEvent.h b/libs/pushstreams/api/celix/PushEvent.h index 8cb639ded..8be0a7a9c 100644 --- a/libs/pushstreams/api/celix/PushEvent.h +++ b/libs/pushstreams/api/celix/PushEvent.h @@ -27,7 +27,7 @@ namespace celix { public: virtual ~PushEvent() = default; - enum class EventType { + enum class EventType : std::uint8_t { DATA, ERROR, CLOSE diff --git a/libs/pushstreams/api/celix/PushStream.h b/libs/pushstreams/api/celix/PushStream.h index 61e1533c0..c674a2d33 100644 --- a/libs/pushstreams/api/celix/PushStream.h +++ b/libs/pushstreams/api/celix/PushStream.h @@ -114,7 +114,7 @@ namespace celix { protected: explicit PushStream(std::shared_ptr& promiseFactory); - enum class State { + enum class State : std::uint8_t { BUILDING, STARTED, CLOSED From 74d9c24ed5be8a708843eb9130edaed07cd36bd8 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sun, 4 Jun 2023 19:17:26 +0200 Subject: [PATCH 12/13] Update coding conventions readme --- documents/development/README.md | 4 ++-- libs/framework/gtest/src/DependencyManagerTestSuite.cc | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/documents/development/README.md b/documents/development/README.md index 7de1cc8b4..806694d43 100644 --- a/documents/development/README.md +++ b/documents/development/README.md @@ -179,7 +179,7 @@ add_library(celix::my_bundle ALIAS my_bundle) - Use `snake_case` for namespace names. - All namespaces should be part of the `celix` namespace. - Aim for a max of 3 levels of namespaces. -- Use a namespace ending with `impl` for implementation details. +- Use a namespace ending with `detail` for implementation details. ### C++ Classes @@ -214,7 +214,7 @@ namespace celix { ### C++ Enums - Use `CamelCase` (starting with a capital) for enum types names. -- Use `enum class` instead of `enum`. +- Use `enum class` instead of `enum` and if possible use `std::int8_t` as base type. - Use `SNAKE_CASE` for enum values without a celix/class prefix. Note that for enum values no prefix is required because enum class values are scoped. diff --git a/libs/framework/gtest/src/DependencyManagerTestSuite.cc b/libs/framework/gtest/src/DependencyManagerTestSuite.cc index 5b312280e..4e2cdd8b3 100644 --- a/libs/framework/gtest/src/DependencyManagerTestSuite.cc +++ b/libs/framework/gtest/src/DependencyManagerTestSuite.cc @@ -650,7 +650,7 @@ TEST_F(DependencyManagerTestSuite, IntermediateStatesDuringInitDeinitStartingAnd //and a required service dependency on "RequiredTestService" with a locking-strategy. class LifecycleComponent { public: - enum class LifecycleMethod { + enum class LifecycleMethod : std::int8_t { None = 0, Init = 1, Start = 2, @@ -1010,7 +1010,7 @@ TEST_F(DependencyManagerTestSuite, UnneededSuspendIsPrevented) { TEST_F(DependencyManagerTestSuite, ExceptionsInLifecycle) { class ExceptionComponent { public: - enum class LifecycleMethod { + enum class LifecycleMethod : std::uint8_t { INIT, START, STOP, From d2a82305e43b0fe3e71b8024a70c899756b06d86 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sun, 4 Jun 2023 19:18:18 +0200 Subject: [PATCH 13/13] Remove include regroup from clang-format file --- .clang-format | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/.clang-format b/.clang-format index 1e86f6120..2766cd9cc 100644 --- a/.clang-format +++ b/.clang-format @@ -11,20 +11,22 @@ KeepEmptyLinesAtTheStartOfBlocks: false PointerAlignment: Left SpacesBeforeTrailingComments: 1 SortIncludes: true -IncludeBlocks: Regroup -IncludeCategories: -#gtest headers - - Regex: '^((<|")gtest/)' - Priority: 1 -#stdlib C++ headers - - Regex: '^<([^.])*>' - Priority: 2 -#external headers - - Regex: '^<([^.])*\.h>' - Priority: 3 -#celix C API - - Regex: '^(<|")celix_.*' - Priority: 4 -#celix C++ API - - Regex: '^(<|")celix/.*' - Priority: 5 +--- +#Include regroup disabled by default. +#IncludeBlocks: Regroup +#IncludeCategories: +##gtest headers +# - Regex: '^((<|")gtest/)' +# Priority: 1 +##stdlib C++ headers +# - Regex: '^<([^.])*>' +# Priority: 2 +##external headers +# - Regex: '^<([^.])*\.h>' +# Priority: 3 +##celix C API +# - Regex: '^(<|")celix_.*' +# Priority: 4 +##celix C++ API +# - Regex: '^(<|")celix/.*' +# Priority: 5