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.

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

Coding - Add clang-format configuration #246

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

dpasukhi
Copy link
Member

@dpasukhi dpasukhi commented Jan 6, 2025

New clang-format configuration added to determinate code style. The default version is 16.
Extend CMake to copy config file to build root.
Method/function separator deprecation:
  In case if function/method has declaration in header,
    definition must not have related comment.
    Only //==== [100 chars] ==== is allowed as a not connected
    separator.
  In case if function/method has  NOT declaration in header,
    definition must have related comment in doxygen style:
    // Descriptions
    // @param
    // @return
    Or just function/method separator:
    //==== [100 chars] ====
  All old separators with no description must be replaced to
    //==== [100 chars] ====
Header file comments:
  Comments in header files still started by //!
    Each public method must to have description in doxygen style. Each parameter must to be descripted.
Source file comments:
  Comments in source file always started by //
  Each internal classes/structures must to be descripted and each static method must to have at least short description in free form or doxygen style.

For example regex, that i was using to update method guards with no description in .cxx and .gxx files:
search:
//==[=]+[\n\r]//[\s\n\r\t]*(?i:function)[\s\n\r\t]*:[\s\n\r\t]*[a-z0-9_:]*[\s\n\r\t]*//[\s\n\r\t]*(?i:purpose)[\s\n\r\t]*:[\s\n\r\t]*//===[=]+
replace
//=================================================================================================\n

Current PR has 3 packages code style updated, chosen randomly to present new changes.
Updated packages(only cxx and hxx files):

  • DE
  • Approx
  • ApproxInt

Used pwsh (powershell) command:

cd src/DE
Get-ChildItem -Recurse -Include *.hxx, *.cxx, *.lxx, *.gxx -Path ../DE, ../Approx, ../ApproxInt | ForEach-Object { clang-format -i -style=file:./../../.clang-format $_.FullName }

_
second commit regex targeted to single line separator with little details:
//==[=]+[\n\r]+//[\s\n\r\t]*(?i:function)[\s\n\r\t]*:[\s\n\r\t]*[a-z0-9_:\t\s]*[\s\n\r\t]*//[\s\n\r\t]*(?i:purpose)[\s\n\r\t]*[a-z0-9_:\s\t.]*//===[=]+
files: src/DE/**, src/Approx/**, src/ApproxInt/**
replace to //=================================================================================================\n
new regex:
//-----[-]+
replace to:
//=================================================================================================\n

@dpasukhi dpasukhi added 2. Enhancement New feature or request 1. Coding Coding rules, trivial changes and misprints labels Jan 6, 2025
@dpasukhi dpasukhi added this to the Release 7.9 milestone Jan 6, 2025
@dpasukhi dpasukhi self-assigned this Jan 6, 2025
@dpasukhi dpasukhi requested review from a team January 6, 2025 19:02
@dpasukhi
Copy link
Member Author

dpasukhi commented Jan 6, 2025

PR must be analyzed by each linear team internally.
Any comments must be sent by email or PR comment.
I will organize internal notification of more members to define single code style.

@dpasukhi dpasukhi changed the base branch from master to IR January 6, 2025 19:04
Copy link
Contributor

@AtheneNoctuaPt AtheneNoctuaPt left a comment

Choose a reason for hiding this comment

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

Hello Dmitrii,
There are some remarks regarding deprecated functions/classes comments still remaining in the code. Since this update is supposed to represent the new style, we should probably fix that to avoid confusion.

src/Approx/Approx_Curve2d.cxx Outdated Show resolved Hide resolved
src/Approx/Approx_Curve3d.cxx Outdated Show resolved Hide resolved
src/Approx/Approx_CurveOnSurface.cxx Outdated Show resolved Hide resolved
src/Approx/Approx_CurveOnSurface.cxx Outdated Show resolved Hide resolved
src/Approx/Approx_CurveOnSurface.cxx Outdated Show resolved Hide resolved
src/Approx/Approx_SweepApproximation.cxx Outdated Show resolved Hide resolved
src/Approx/Approx_SweepApproximation.cxx Outdated Show resolved Hide resolved
src/ApproxInt/ApproxInt_KnotTools.cxx Outdated Show resolved Hide resolved
src/ApproxInt/ApproxInt_SvSurfaces.cxx Outdated Show resolved Hide resolved
src/DE/DE_ShapeFixParameters.hxx Outdated Show resolved Hide resolved
dpasukhi added a commit that referenced this pull request Jan 7, 2025
New clang-format configuration added to determinate code style.
The default version is 16.
Extend CMake to copy config file to build root.
Method/function separator deprecation:
  In case if function/method has declaration in header,
    definition must not have related comment.
    Only //==== [100 chars] ====// is allowed as a not connected
    separator.
  In case if function/method has  NOT declaration in header,
    definition must have related comment in doxygen style:
    // Descriptions
    // @param
    // @return
    Or just function/method separator:
    //==== [100 chars] ====//
  All old separators with no description must be replaced to
    //==== [100 chars] ====//
@dpasukhi
Copy link
Member Author

dpasukhi commented Jan 7, 2025

Can be cloned from the upstream repo for local checking:
https://github.com/Open-Cascade-SAS/OCCT/tree/clang-format

Copy link

Choose a reason for hiding this comment

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

as proposial can be BreakConstructorInitializers: BeforeColon:
image
BreakInheritanceList: BeforeColon
image
BreakBeforeTernaryOperators: true
image
BreakBeforeBinaryOperators: NonAssignment

image
the provided changes can be useful in long expressions (sometimes I split screen, and right part of page can be hidden, but symbols like :,&&,? will be on begin of row, which allow to don't do additional scroll, to read them). Another argument when we try to comment the row to make some checks - in this case we need to remove symbol from another row which can be more time consuming and not convenient. So from my point of view these changes are more representative and usable

Copy link

@vttrtp vttrtp Jan 8, 2025

Choose a reason for hiding this comment

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

maybe also SpaceAfterTemplateKeyword: false
to don't produce extra space:

template <typename T> -> template<typename T> (stl style)

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is my opinion and it is not final. I would be glad if someone else will be able to share their view.
_
BreakConstructorInitializers: AfterColon used to make all arguments in the same condition to fast reorder and manipulate. If we will have : in the same line it will give special attention to first field. And based on some #ifdef cases fields can be initialize differently. So, I would recommend to have all initializations to have the same logic and style.
From my side I would recommend: AfterColon
_
BreakInheritanceList: BeforeColon I would avoid with the same idea as first option related to have the same attention to any inheritance classes. What about choose AfterColon? It make all parents in the same level and helps to clear read the logic. BUT OCCT RTTI mostly avoid multiple inheritance and we usually have only 1 parent. Based on it we can use AfterComma.
From my side I would recommend: AfterColon
_
BreakBeforeBinaryOperators: NonAssignment. Yes, in case of git diff or splitting screen it can helps to avoid some extra actions to read code. But in the same time it will leads to difficulties to understand the operation type because the operators are randomly located. The current colon limitation is 100 symbols. For sure it is not comfortable for splitting, but still enough for general code). Usually so complex operations are short in line.
So, benefit - clear view on tiny screen.
Misadventures - less comfort to read code in normal development time.
As for a code commenting - IDE helps to comment selected peace of code with /**/. In that case developer truly will be sure which peace of operation he will comment. (commenting || or && can leads to different results).
From my side I would recommend: None

Copy link

Choose a reason for hiding this comment

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

IndentPPDirectives: BeforeHash
will look more readable:
image

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe also SpaceAfterTemplateKeyword: false
to don't produce extra space:

Yes, I agree. If someone have another view, welcome.

Copy link
Member Author

Choose a reason for hiding this comment

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

BeforeComma in context of (1) and (2) are making attention for the developer on the some not useful syntax.
In that context : or , has no so much impact on the clarity of the code.
What I mean - that symbol usually ignored by developer when looking into code. I guess better to have attension on fields itself then have "symbol_space_field".
In current version we have clear view on the fields and their position then on some syntax. All syntax stuff put on the out-of-attention position (first line and end of the current line).
From my side I would recommend to stay with AfterColon.
(2) AfterColon equal with AfterComma in our context in case if class have enough space to locate. So for average cases we will have no see differences, but in rare cases AfterComma will have more clarity.

Choose a reason for hiding this comment

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

I just wrote a short script to search on Github what others are using in .clang-format. As the API only returns a maximum number of 100 search results, I only created a statistic for that number. Not all found .clang-format files had the specific options set:

BreakInheritanceList Statistics:

Option Usage
BeforeColon 64.3%
AfterColon 15.5%
BeforeComma 17.9%
AfterComma 2.4%

BreakConstructorInitializers Statistics:

Option Usage
BeforeColon 56.2%
BeforeComma 27.0%
AfterColon 16.9%

Copy link

@vttrtp vttrtp Jan 9, 2025

Choose a reason for hiding this comment

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

Additional argument for BeforeColon that dots on the beginning help you to understand that in this place we start to define superclasses or parameters and nothing above

Copy link
Member Author

Choose a reason for hiding this comment

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

The GitHub research about the usage of some parameters in clang-format doesn't have a direct equivalent for the "comfort" style in our project.
The idea of .clang-format is to use the default style and then update some project-related options that have specific benefits for their team, their product, and its lifecycle.
I checked the GNU and Microsoft coding styles, and they do indeed use BeforeColon.
In that case, I would agree to use it, based on common practice.
I agree with that approach and will remove the override to use the default Microsoft style.
_
Additionally, let me share the main purpose of the new code style formatting:
Most GitHub C++ projects are small pet-projects. OCCT, however, is a huge codebase with code that hasn't been modified by humans for decades.
But that code is still read by humans from time to time. That is why the main direction for the new formatting style is clarity for reading and review.

Copy link
Member Author

Choose a reason for hiding this comment

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

BeforeColon will have extra 2 spaces at the beginning. But oke, if 2 of you and microsoft have that I agree with that price.
You can check last modifications.

IndentCaseLabels: true
IndentWidth: 2
IndentWrappedFunctionNames: true
PackConstructorInitializers: Never
Copy link

@vttrtp vttrtp Jan 8, 2025

Choose a reason for hiding this comment

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

maybe there is sense to remove this parameter from config (default PackConstructorInitializers: CurrentLine)to have short constructors such as:

Constructor() : a(), b()

Copy link
Member Author

Choose a reason for hiding this comment

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

Making inline initialization list leads to possible issue with incorrect ordering and complicity with reorder. Making attentions on initialization order is very important in C++ because we have depends on class packing and location in memory.
From my side i would keep the current style.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree here with Dmitrii. For me personally it is somewhat easier to recognize correct/incorrect order when initializers are always ordered in the same way as respective fields are declared in class - from top to down. Besides, while the example with just a(), b() looks pretty, realistically majority of classes in our codebase would look like:

Constructor(int theArgument) : myFirstField(), myData(theArgument), myOtherField(), myFlag(true)

which is harder to read, in my opinion, compared to

Constructor(int theArgument)
: myFirstField(),
  myData(theArgument),
  myOtherField(),
  myFlag(true)

Copy link

Choose a reason for hiding this comment

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

Actually in microsoft version of stl they use spaces after template keyword but in libstd no
image
image

Copy link
Member Author

@dpasukhi dpasukhi Jan 8, 2025

Choose a reason for hiding this comment

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

GNU/Microsoft coding style. We are using Microsoft by default.
So, only fields that on .clang-format they are creating differences from the some common styles.
The main attention in modification are for packing, sorting and splitting. Spaces better to keep as in default style. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

GNU/Microsoft coding style. We are using Microsoft by default. So, only fields that on .clang-format they are creating differences from the some common styles. The main attention in modification are for packing, sorting and splitting. Spaces better to keep as in default style. What do you think?

For me, any will do.

@dpasukhi
Copy link
Member Author

If no more suggestions or recommendations - will be merge today evening into IR.
19.01 will be merged to "master"

New clang-format configuration added to determinate code style.
The default version is 16.
Extend CMake to copy config file to build root.
Method/function separator deprecation:
  In case if function/method has declaration in header,
    definition must not have related comment.
    Only //==== [100 chars] ==== is allowed as a not connected
    separator.
  In case if function/method has  NOT declaration in header,
    definition must have related comment in doxygen style:
    // Descriptions
    // @param
    // @return
    Or just function/method separator:
    //==== [100 chars] ====
  All old separators with no description must be replaced to
    //==== [100 chars] ====
@dpasukhi dpasukhi merged commit fe1382f into Open-Cascade-SAS:IR Jan 15, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1. Coding Coding rules, trivial changes and misprints 2. Enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants