Feature/coding conventions #544

pnoltes · 2023-05-06T09:37:37Z

This PR introduces an Apache Celix Coding Convention document along with a .clang-format file.

These additions aim to streamline Celix development by promoting more readable and maintainable code.

Please consider this PR as a proposal and feel free to contribute suggestions for changes. However, if this PR is merged, compliance with the coding conventions and format style is encouraged during pull request reviews.

For this particular PR, I would appreciate receiving some more reviews to ensure that the proposed changes are widely accepted by the community.

…entions

codecov-commenter · 2023-05-06T09:39:08Z

Codecov Report

Merging #544 (3a0661c) into master (f9ada53) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 3a0661c differs from pull request most recent head 546490e. Consider uploading reports for the commit 546490e to get more accurate results

@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
- Coverage   78.03%   78.02%   -0.01%     
==========================================
  Files         229      229              
  Lines       35016    35016              
==========================================
- Hits        27324    27322       -2     
- Misses       7692     7694       +2

Impacted Files	Coverage Δ
...xx_remote_services/admin/src/RemoteServiceAdmin.cc	`78.70% <ø> (ø)`
libs/framework/include/celix/Bundle.h	`100.00% <ø> (ø)`
libs/framework/include/celix/ServiceRegistration.h	`96.63% <ø> (ø)`
libs/framework/include/celix/Trackers.h	`92.00% <ø> (ø)`
libs/framework/include/celix/dm/Component.h	`94.44% <ø> (ø)`
libs/pushstreams/api/celix/PushEvent.h	`75.00% <ø> (ø)`
libs/pushstreams/api/celix/PushStream.h	`95.12% <ø> (ø)`
libs/utils/include/celix/Filter.h	`100.00% <ø> (ø)`

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

documents/development/README.md

rlenferink · 2023-05-11T18:45:18Z

documents/development/README.md

+  - 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


Don't this and the line above it contradict each other?

First sentence: For the Apache Celix framework and utils lib only => I read this as "header-only C++ files are only allowed for the Apache Celix framework and utils lib"

Prefer header-only C++ files for the Apache Celix libraries => I read this as "header-only C++ files are preferred for all Apache Celix libraries".

I think the former item refers to Celix::framework and Celix::utils, while the latter item refers to other libraries.

I moved the two lines to ### C++ Libaries and tried to clarify them a bit more.

rlenferink · 2023-05-11T18:46:28Z

documents/development/README.md

+- 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.


What do we do when switching to newer C++ versions? Update the coding guidelines? Do we need to mention the C++ version in the coding guidelines? Is that not more of a technical 'setting'?

IMO updating the guidelines, this should be a living document.

documents/development/README.md

rlenferink · 2023-05-11T19:05:15Z

documents/development/README.md

+## Benchmarking
+
+- When needed, use benchmarking to measure performance.
+- Use the Google Benchmark framework for benchmarking.


Why only aim at using Google Benchmark? If someone finds a better tool to do the job, that's up to them IMO (up to the user).

To keep the external libs a bit more manageable. If there is a good reason to use a different benchmark lib, I see no problem with updating the guidlines.

rlenferink · 2023-05-11T19:07:15Z

/cc @wopss maybe you will find this an interesting PR as well ;)

wopss · 2023-05-12T17:30:14Z

documents/development/README.md

+
+- 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.


How deep can a namespace be? Can I do something like celix::custom_implemntation_detail::some_other_namespace or just a celix::detail is more than enough for an implementation specific class?

I think it is worth mentioning if you prefer the namespaces to be max 2 level deep, e.g. celix:: and celix::detail. I think everything that is "implementation" specific and not part of the public API should be in celix::detail (max. 2 levels).

I prefer impl over detail for implementation details, because detail is more logical to be used broader than for implementation details .

That is also good. But I strongly suggest detail for private implementation details.

The reason I suggest detail is because it is widely used in C++, most libraries (e.g. boost, fmtlib, spdlog) are using the following "convention":

libname:: - This is for public API, which includes everything the user of the library is intended to interact with.

libname::detail - This is for implementation details that are not meant to be seen or used directly by the user of the library.

documents/development/README.md

wopss · 2023-05-12T17:34:16Z

documents/development/README.md

+- 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.
+


Is there a preference for the base type of an enum? E.g.:

enum class ServiceRegistrationState : std::int8_t

No not yet. Is there a good reason to do this?

Mostly storage space. By default enum class is 4 bytes, if not all values are used then the rest of the space is wasted.

Actual space depends of course on alignment, but if there are no real downsides to using a std::int8_t, I think this is a good idea. I will update this.

documents/development/README.md

wopss · 2023-05-12T18:06:09Z

.clang-format

+ConstructorInitializerIndentWidth: 4
+Cpp11BracedListStyle: true
+DerivePointerAlignment: false
+IncludeBlocks: Regroup


I looked into clang-format 15 and it has the following for LLVM:

IncludeCategories: - Regex: '^"(llvm|llvm-c|clang|clang-c)/' Priority: 2 CaseSensitive: false - Regex: '^(<|"(gtest|gmock|isl|json)/)' Priority: 3 CaseSensitive: false - Regex: '.*' Priority: 1 CaseSensitive: false

which will result in STL headers being at the end of the include list, is this something that is preferred? I would recommend something like (based on LLVM configuration) this in a header or source file:

#include <HEADER_OF_CPP_IF_NEEDED> #include <STL_HEADER_1> #include <STL_HEADER_2> #include <OTHER_LIB_HEADER_1> #include <OTHER_LIB_HEADER_2> #include <THIRD_PARTY_HEADERS>

I would even set this to Preserved and let the developer decide, or create a custom IncludeCategories block.

I personally are not a fan of organizing headers manually, so I prefer adding a IncludeCategories section.

AFAIK, there is a case where manually organizing headers can be useful: for each .h/.c pairs, e.g., celix_bundle_cache.h(.c), always include the header file (celix_bundle_cache.h) in the first line of the non-comment part of the source file (celix_bundle_cache.c) to make sure the header file is self-contained, which means it can be used alone. Self-contained header file is really nice.

clang-format does take into account. So when you run formatting over a foo.c source file and there is a include of foo.h this will be included first.

This should even be extendable with the IncludeIsMainRegex option, but I could not get that to work.

I've consistently encountered problems with this setting while developing libraries, particularly when using angle brackets (<>) to include header files within the library. However, these problems don't seem to occur when using double quotes ("").

E.g.:

// lib/detail/header1.cpp #include <lib/detail/header1.hpp> #include <lib/header2.hpp> #include <lib/header3.hpp> #include <memory> #include <spdlog/spdlog.hpp>

will get reorganized in:

// lib/detail/header1.cpp #include <memory> #include <lib/header2.hpp> #include <lib/header3.hpp> #include <lib/detail/header1.hpp> // This header should be at the top of the file in my opinion. #include <spdlog/spdlog.hpp>

while, the following:

// lib/detail/header1.cpp #include "lib/detail/header1.hpp" #include "lib/header2.hpp" #include "lib/header3.hpp" #include <memory> #include <spdlog/spdlog.hpp>

will get reorganized in:

// lib/detail/header1.cpp #include "lib/detail/header1.hpp" #include <memory> #include "lib/header2.hpp" #include "lib/header3.hpp" #include <spdlog/spdlog.hpp>

I purpose to not regroup includes header for now.

I will leave this as comments, so that it can be used if desired on case per case basis.
If needed we can revisit this at a later stage.

.clang-format

wopss · 2023-05-12T18:15:00Z

Good job for writing all of these!

Since some things can be overseen in reviews I suggest to setup a workflow to check the formatting of new C++ code. If this is a good idea I can submit a PR for it.

pnoltes · 2023-05-16T10:19:12Z

Good job

Thanks.
Yes I think that is a good idea, ideally this only applies for adjusted code and new source files but I would be interested in workflow that can help us with ensuring consistent formatting.

…entions

Also updates some source files to test and show clang-format header grouping result.

PengZheng

LGTM

…entions # Conflicts: # libs/framework/src/bundle_archive.c

…entions # Conflicts: # libs/utils/include/celix/Filter.h

PengZheng · 2023-06-10T12:50:46Z

If no further comments, I suggest give the clang-format file a try. @xuzhenbao

pnoltes · 2023-06-11T13:06:43Z

While I would prefer additional reviewers, this pull request has remained open for an extended period, so I'm going to merge it now. If there are any suggestions for updating the coding conventions, these can be implemented in a separate pull request.

pnoltes added 7 commits April 15, 2023 20:10

Add initial coding conventions markdown document

c3c5772

Update coding convention doc

d188dd4

Add .clang-format file

7ec7c39

Update the naming convention documentation

65750a5

Merge remote-tracking branch 'origin/master' into feature/coding_conv…

b38d080

…entions

Update coding convention doc (#442)

a565eff

Update clang-format file (#508)

88ea127

pnoltes requested review from PengZheng, rlenferink, xuzhenbao and stegemr May 6, 2023 09:38

PengZheng reviewed May 8, 2023

View reviewed changes

documents/development/README.md Outdated Show resolved Hide resolved

Apply suggestions from code review

b688dc0

rlenferink reviewed May 11, 2023

View reviewed changes

documents/development/README.md Outdated Show resolved Hide resolved

rlenferink reviewed May 11, 2023

View reviewed changes

documents/development/README.md Outdated Show resolved Hide resolved

rlenferink reviewed May 11, 2023

View reviewed changes

wopss reviewed May 12, 2023

View reviewed changes

This was linked to issues May 13, 2023

Bundle binary footprint optimization and symbol visibility. #442

Closed

Add clang-format style for the Apache Celix project #508

Closed

pnoltes added 4 commits May 16, 2023 12:20

Applied some of the review comments for PR #544

ef45f41

Merge remote-tracking branch 'origin/master' into feature/coding_conv…

49ef5c2

…entions

Update dev guidelines and clang-format

8ed95f2

Also updates some source files to test and show clang-format header grouping result.

Remove redundant clang-format options

ec71872

pnoltes requested review from wopss and PengZheng May 22, 2023 20:45

PengZheng approved these changes May 23, 2023

View reviewed changes

pnoltes added 4 commits June 4, 2023 19:17

Use std::int8_t as enum class base

e32f4d5

Update coding conventions readme

74d9c24

Remove include regroup from clang-format file

d2a8230

Merge remote-tracking branch 'origin/master' into feature/coding_conv…

be88ea0

…entions # Conflicts: # libs/framework/src/bundle_archive.c

pnoltes requested a review from rlenferink June 4, 2023 17:19

wopss approved these changes Jun 10, 2023

View reviewed changes

Merge remote-tracking branch 'origin/master' into feature/coding_conv…

546490e

…entions # Conflicts: # libs/utils/include/celix/Filter.h

pnoltes merged commit ab2cc5e into master Jun 11, 2023

pnoltes deleted the feature/coding_conventions branch June 11, 2023 13:06

PengZheng mentioned this pull request Jul 16, 2023

RSA, Polyglot and Descriptor Generation #590

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/coding conventions #544

Feature/coding conventions #544

pnoltes commented May 6, 2023

codecov-commenter commented May 6, 2023 •

edited

Loading

rlenferink May 11, 2023

PengZheng May 12, 2023 •

edited

Loading

pnoltes May 16, 2023

rlenferink May 11, 2023

pnoltes May 16, 2023

rlenferink May 11, 2023

pnoltes May 16, 2023

rlenferink commented May 11, 2023

wopss May 12, 2023

pnoltes May 16, 2023

wopss May 25, 2023

wopss May 12, 2023

pnoltes May 16, 2023

wopss May 25, 2023

pnoltes Jun 4, 2023

wopss May 12, 2023

pnoltes May 16, 2023 •

edited

Loading

PengZheng May 17, 2023

pnoltes May 22, 2023

wopss May 25, 2023 •

edited

Loading

pnoltes Jun 4, 2023

wopss commented May 12, 2023

pnoltes commented May 16, 2023

PengZheng left a comment

PengZheng commented Jun 10, 2023

pnoltes commented Jun 11, 2023

Feature/coding conventions #544

Feature/coding conventions #544

Conversation

pnoltes commented May 6, 2023

codecov-commenter commented May 6, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

PengZheng May 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rlenferink commented May 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnoltes May 16, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wopss May 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wopss commented May 12, 2023

pnoltes commented May 16, 2023

PengZheng left a comment

Choose a reason for hiding this comment

PengZheng commented Jun 10, 2023

pnoltes commented Jun 11, 2023

codecov-commenter commented May 6, 2023 •

edited

Loading

PengZheng May 12, 2023 •

edited

Loading

pnoltes May 16, 2023 •

edited

Loading

wopss May 25, 2023 •

edited

Loading