Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/coding conventions #544

Merged
merged 17 commits into from
Jun 11, 2023
Merged

Feature/coding conventions #544

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
c3c5772
Add initial coding conventions markdown document
pnoltes Apr 15, 2023
d188dd4
Update coding convention doc
pnoltes May 5, 2023
7ec7c39
Add .clang-format file
pnoltes May 5, 2023
65750a5
Update the naming convention documentation
pnoltes May 5, 2023
b38d080
Merge remote-tracking branch 'origin/master' into feature/coding_conv…
pnoltes May 6, 2023
a565eff
Update coding convention doc (#442)
pnoltes May 6, 2023
88ea127
Update clang-format file (#508)
pnoltes May 6, 2023
b688dc0
Apply suggestions from code review
PengZheng May 9, 2023
ef45f41
Applied some of the review comments for PR #544
pnoltes May 16, 2023
49ef5c2
Merge remote-tracking branch 'origin/master' into feature/coding_conv…
pnoltes May 16, 2023
8ed95f2
Update dev guidelines and clang-format
pnoltes May 16, 2023
ec71872
Remove redundant clang-format options
pnoltes May 22, 2023
e32f4d5
Use std::int8_t as enum class base
pnoltes Jun 4, 2023
74d9c24
Update coding conventions readme
pnoltes Jun 4, 2023
d2a8230
Remove include regroup from clang-format file
pnoltes Jun 4, 2023
be88ea0
Merge remote-tracking branch 'origin/master' into feature/coding_conv…
pnoltes Jun 4, 2023
546490e
Merge remote-tracking branch 'origin/master' into feature/coding_conv…
pnoltes Jun 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions .clang-format
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
BasedOnStyle: LLVM
pnoltes marked this conversation as resolved.
Show resolved Hide resolved
IndentWidth: 4
AllowShortIfStatementsOnASingleLine: false
AlwaysBreakTemplateDeclarations: Yes
BinPackArguments: false
BinPackParameters: false
BreakBeforeBraces: Attach
ColumnLimit: 120
ConstructorInitializerIndentWidth: 4
Cpp11BracedListStyle: true
DerivePointerAlignment: false
IncludeBlocks: Regroup
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@pnoltes pnoltes May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@wopss wopss May 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

KeepEmptyLinesAtTheStartOfBlocks: false
NamespaceIndentation: None
PointerAlignment: Right
pnoltes marked this conversation as resolved.
Show resolved Hide resolved
ReflowComments: true
SortIncludes: false
pnoltes marked this conversation as resolved.
Show resolved Hide resolved
SpaceAfterCStyleCast: false
SpaceBeforeAssignmentOperators: true
SpaceBeforeParens: ControlStatements
SpaceInEmptyParentheses: false
SpacesBeforeTrailingComments: 1
SpacesInAngles: Never
SpacesInCStyleCastParentheses: false
SpacesInParentheses: false
SpacesInSquareBrackets: false
AlignEscapedNewlines: Right
pnoltes marked this conversation as resolved.
Show resolved Hide resolved
1 change: 1 addition & 0 deletions documents/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Loading