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

Coding - Add clang-format configuration #246

Merged
merged 1 commit into from
Jan 15, 2025
Merged

Coding - Add clang-format configuration #246

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
40 changes: 40 additions & 0 deletions .clang-format
View file
Open in desktop
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 •
edited
Loading

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.

Copy link

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 •
edited
Loading

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# clang-format configuration file trying to apply OCCT coding style
#
# Clang formatting rules: https://clang.llvm.org/docs/ClangFormatStyleOptions.html
# The clang-format npm package (https://github.com/angular/clang-format) uses
# a pre-built clang-format.exe from http://llvm.org/builds/
#
# We use defaults from the Microsoft style
BasedOnStyle: Microsoft
#
# Style options
AllowAllParametersOfDeclarationOnNextLine: false
AllowAllArgumentsOnNextLine: false
AlignAfterOpenBracket: Align
AlignConsecutiveAssignments: Consecutive
AlignConsecutiveDeclarations: Consecutive
AlignTrailingComments: true
AllowShortFunctionsOnASingleLine: Inline
AlwaysBreakTemplateDeclarations: Yes
BinPackArguments: false
BinPackParameters: false
BreakBeforeBinaryOperators: NonAssignment
BreakBeforeTernaryOperators: true
ColumnLimit: 100
ContinuationIndentWidth: 2
IndentCaseLabels: true
IndentPPDirectives: BeforeHash
IndentWidth: 2
IndentWrappedFunctionNames: true
PackConstructorInitializers: Never
Copy link

@vttrtp vttrtp Jan 8, 2025 •
edited
Loading

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 •
edited
Loading

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.

PointerAlignment: Left
ReferenceAlignment: Left
SeparateDefinitionBlocks: Always
SortIncludes: true
UseTab: Never
#
# OCCT specific settings
StatementMacros:
- Standard_FALLTHROUGH
TypenameMacros:
- Handle
3 changes: 3 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -723,6 +723,9 @@ endif()
# the name of the project
project (OCCT)

# copying clang-format file to the root of the project
file(COPY ${CMAKE_SOURCE_DIR}/.clang-format DESTINATION ${CMAKE_SOURCE_DIR})

# Get all used variables: OS_WITH_BIT, COMPILER
OCCT_MAKE_OS_WITH_BITNESS()
OCCT_MAKE_COMPILER_SHORT_NAME()
Expand Down
1 change: 0 additions & 1 deletion src/Approx/Approx_Array1OfAdHSurface.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,4 @@

typedef NCollection_Array1<Handle(Adaptor3d_Surface)> Approx_Array1OfAdHSurface;


#endif
3 changes: 1 addition & 2 deletions src/Approx/Approx_Array1OfGTrsf2d.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,9 @@
#ifndef Approx_Array1OfGTrsf2d_HeaderFile
#define Approx_Array1OfGTrsf2d_HeaderFile

#include <gp_GTrsf2d.hxx>
#include <NCollection_Array1.hxx>
#include <gp_GTrsf2d.hxx>

typedef NCollection_Array1<gp_GTrsf2d> Approx_Array1OfGTrsf2d;


#endif
209 changes: 115 additions & 94 deletions src/Approx/Approx_Curve2d.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
// Alternatively, this file may be used under the terms of Open CASCADE
// commercial license or contractual agreement.


#include <Adaptor2d_Curve2d.hxx>
#include <AdvApprox_ApproxAFunction.hxx>
#include <AdvApprox_PrefAndRec.hxx>
Expand All @@ -23,150 +22,172 @@
#include <Precision.hxx>
#include <TColgp_Array1OfPnt2d.hxx>

//=======================================================================
//class : Approx_Curve2d_Eval
//purpose: evaluator class for approximation
//=======================================================================
//=================================================================================================

class Approx_Curve2d_Eval : public AdvApprox_EvaluatorFunction
{
public:
Approx_Curve2d_Eval (const Handle(Adaptor2d_Curve2d)& theFunc,
Standard_Real First, Standard_Real Last)
: fonct(theFunc) { StartEndSav[0] = First; StartEndSav[1] = Last; }

virtual void Evaluate (Standard_Integer *Dimension,
Standard_Real StartEnd[2],
Standard_Real *Parameter,
Standard_Integer *DerivativeRequest,
Standard_Real *Result, // [Dimension]
Standard_Integer *ErrorCode);

private:
public:
Approx_Curve2d_Eval(const Handle(Adaptor2d_Curve2d)& theFunc,
Standard_Real First,
Standard_Real Last)
: fonct(theFunc)
{
StartEndSav[0] = First;
StartEndSav[1] = Last;
}

virtual void Evaluate(Standard_Integer* Dimension,
Standard_Real StartEnd[2],
Standard_Real* Parameter,
Standard_Integer* DerivativeRequest,
Standard_Real* Result, // [Dimension]
Standard_Integer* ErrorCode);

private:
Handle(Adaptor2d_Curve2d) fonct;
Standard_Real StartEndSav[2];
Standard_Real StartEndSav[2];
};

void Approx_Curve2d_Eval::Evaluate (Standard_Integer *Dimension,
Standard_Real StartEnd[2],
Standard_Real *Param, // Parameter at which evaluation
Standard_Integer *Order, // Derivative Request
Standard_Real *Result,// [Dimension]
Standard_Integer *ErrorCode)
void Approx_Curve2d_Eval::Evaluate(Standard_Integer* Dimension,
Standard_Real StartEnd[2],
Standard_Real* Param, // Parameter at which evaluation
Standard_Integer* Order, // Derivative Request
Standard_Real* Result, // [Dimension]
Standard_Integer* ErrorCode)
{
*ErrorCode = 0;
*ErrorCode = 0;
Standard_Real par = *Param;

// Dimension is incorrect
if (*Dimension!=2) {
// Dimension is incorrect
if (*Dimension != 2)
{
*ErrorCode = 1;
}
// Parameter is incorrect
if ( par < StartEnd[0] || par > StartEnd[1] ) {
// Parameter is incorrect
if (par < StartEnd[0] || par > StartEnd[1])
{
*ErrorCode = 2;
}
if(StartEnd[0] != StartEndSav[0] || StartEnd[1]!= StartEndSav[1])
{
fonct = fonct->Trim(StartEnd[0],StartEnd[1],Precision::PConfusion());
StartEndSav[0]=StartEnd[0];
StartEndSav[1]=StartEnd[1];
}
if (StartEnd[0] != StartEndSav[0] || StartEnd[1] != StartEndSav[1])
{
fonct = fonct->Trim(StartEnd[0], StartEnd[1], Precision::PConfusion());
StartEndSav[0] = StartEnd[0];
StartEndSav[1] = StartEnd[1];
}
gp_Pnt2d pnt;
gp_Vec2d v1, v2;

switch (*Order) {
case 0:
pnt = fonct->Value(par);
Result[0] = pnt.X();
Result[1] = pnt.Y();
break;
case 1:
fonct->D1(par, pnt, v1);
Result[0] = v1.X();
Result[1] = v1.Y();
break;
case 2:
fonct->D2(par, pnt, v1, v2);
Result[0] = v2.X();
Result[1] = v2.Y();
break;
default:
Result[0] = Result[1] = 0.;
*ErrorCode = 3;
break;
switch (*Order)
{
case 0:
pnt = fonct->Value(par);
Result[0] = pnt.X();
Result[1] = pnt.Y();
break;
case 1:
fonct->D1(par, pnt, v1);
Result[0] = v1.X();
Result[1] = v1.Y();
break;
case 2:
fonct->D2(par, pnt, v1, v2);
Result[0] = v2.X();
Result[1] = v2.Y();
break;
default:
Result[0] = Result[1] = 0.;
*ErrorCode = 3;
break;
}
}

Approx_Curve2d::Approx_Curve2d(const Handle(Adaptor2d_Curve2d)& C2D,const Standard_Real First,const Standard_Real Last,const Standard_Real TolU,const Standard_Real TolV,const GeomAbs_Shape Continuity,const Standard_Integer MaxDegree,const Standard_Integer MaxSegments)
Approx_Curve2d::Approx_Curve2d(const Handle(Adaptor2d_Curve2d)& C2D,
const Standard_Real First,
const Standard_Real Last,
const Standard_Real TolU,
const Standard_Real TolV,
const GeomAbs_Shape Continuity,
const Standard_Integer MaxDegree,
const Standard_Integer MaxSegments)
{
C2D->Trim(First,Last,Precision::PConfusion());

Standard_Integer Num1DSS=2, Num2DSS=0, Num3DSS=0;
Handle(TColStd_HArray1OfReal) TwoDTolNul, ThreeDTolNul;
Handle(TColStd_HArray1OfReal) OneDTol = new TColStd_HArray1OfReal(1,Num1DSS);
OneDTol->ChangeValue(1) = TolU;
OneDTol->ChangeValue(2) = TolV;
Standard_Integer NbInterv_C2 = C2D->NbIntervals(GeomAbs_C2);
TColStd_Array1OfReal CutPnts_C2(1, NbInterv_C2+1);
C2D->Trim(First, Last, Precision::PConfusion());

Standard_Integer Num1DSS = 2, Num2DSS = 0, Num3DSS = 0;
Handle(TColStd_HArray1OfReal) TwoDTolNul, ThreeDTolNul;
Handle(TColStd_HArray1OfReal) OneDTol = new TColStd_HArray1OfReal(1, Num1DSS);
OneDTol->ChangeValue(1) = TolU;
OneDTol->ChangeValue(2) = TolV;

Standard_Integer NbInterv_C2 = C2D->NbIntervals(GeomAbs_C2);
TColStd_Array1OfReal CutPnts_C2(1, NbInterv_C2 + 1);
C2D->Intervals(CutPnts_C2, GeomAbs_C2);
Standard_Integer NbInterv_C3 = C2D->NbIntervals(GeomAbs_C3);
TColStd_Array1OfReal CutPnts_C3(1, NbInterv_C3+1);
Standard_Integer NbInterv_C3 = C2D->NbIntervals(GeomAbs_C3);
TColStd_Array1OfReal CutPnts_C3(1, NbInterv_C3 + 1);
C2D->Intervals(CutPnts_C3, GeomAbs_C3);

AdvApprox_PrefAndRec CutTool(CutPnts_C2,CutPnts_C3);
AdvApprox_PrefAndRec CutTool(CutPnts_C2, CutPnts_C3);

myMaxError2dU = 0;
myMaxError2dV = 0;

Approx_Curve2d_Eval ev (C2D, First, Last);
AdvApprox_ApproxAFunction aApprox (Num1DSS, Num2DSS, Num3DSS,
OneDTol, TwoDTolNul, ThreeDTolNul,
First, Last, Continuity,
MaxDegree, MaxSegments,
ev, CutTool);

myIsDone = aApprox.IsDone();
Approx_Curve2d_Eval ev(C2D, First, Last);
AdvApprox_ApproxAFunction aApprox(Num1DSS,
Num2DSS,
Num3DSS,
OneDTol,
TwoDTolNul,
ThreeDTolNul,
First,
Last,
Continuity,
MaxDegree,
MaxSegments,
ev,
CutTool);

myIsDone = aApprox.IsDone();
myHasResult = aApprox.HasResult();

if (myHasResult) {
TColgp_Array1OfPnt2d Poles2d(1,aApprox.NbPoles());
TColStd_Array1OfReal Poles1dU(1,aApprox.NbPoles());

if (myHasResult)
{
TColgp_Array1OfPnt2d Poles2d(1, aApprox.NbPoles());
TColStd_Array1OfReal Poles1dU(1, aApprox.NbPoles());
aApprox.Poles1d(1, Poles1dU);
TColStd_Array1OfReal Poles1dV(1,aApprox.NbPoles());
TColStd_Array1OfReal Poles1dV(1, aApprox.NbPoles());
aApprox.Poles1d(2, Poles1dV);
for(Standard_Integer i = 1; i <= aApprox.NbPoles(); i++)
for (Standard_Integer i = 1; i <= aApprox.NbPoles(); i++)
Poles2d.SetValue(i, gp_Pnt2d(Poles1dU.Value(i), Poles1dV.Value(i)));

Handle(TColStd_HArray1OfReal) Knots = aApprox.Knots();
Handle(TColStd_HArray1OfInteger) Mults = aApprox.Multiplicities();
Standard_Integer Degree = aApprox.Degree();
myCurve = new Geom2d_BSplineCurve(Poles2d, Knots->Array1(), Mults->Array1(), Degree);
Handle(TColStd_HArray1OfReal) Knots = aApprox.Knots();
Handle(TColStd_HArray1OfInteger) Mults = aApprox.Multiplicities();
Standard_Integer Degree = aApprox.Degree();
myCurve = new Geom2d_BSplineCurve(Poles2d, Knots->Array1(), Mults->Array1(), Degree);
myMaxError2dU = aApprox.MaxError(1, 1);
myMaxError2dV = aApprox.MaxError(1, 2);
}
}
}

Standard_Boolean Approx_Curve2d::IsDone() const
Standard_Boolean Approx_Curve2d::IsDone() const
{
return myIsDone;
}

Standard_Boolean Approx_Curve2d::HasResult() const
Standard_Boolean Approx_Curve2d::HasResult() const
{
return myHasResult;
}

Handle(Geom2d_BSplineCurve) Approx_Curve2d::Curve() const
Handle(Geom2d_BSplineCurve) Approx_Curve2d::Curve() const
{
return myCurve;
}

Standard_Real Approx_Curve2d::MaxError2dU() const
Standard_Real Approx_Curve2d::MaxError2dU() const
{
return myMaxError2dU;
}

Standard_Real Approx_Curve2d::MaxError2dV() const
Standard_Real Approx_Curve2d::MaxError2dV() const
{
return myMaxError2dV;
}
35 changes: 19 additions & 16 deletions src/Approx/Approx_Curve2d.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,37 +18,40 @@
#define _Approx_Curve2d_HeaderFile

#include <Adaptor2d_Curve2d.hxx>
#include <GeomAbs_Shape.hxx>
#include <Geom2d_BSplineCurve.hxx>
#include <GeomAbs_Shape.hxx>

//! Makes an approximation for HCurve2d from Adaptor3d
class Approx_Curve2d
class Approx_Curve2d
{
public:

DEFINE_STANDARD_ALLOC


Standard_EXPORT Approx_Curve2d(const Handle(Adaptor2d_Curve2d)& C2D, const Standard_Real First, const Standard_Real Last, const Standard_Real TolU, const Standard_Real TolV, const GeomAbs_Shape Continuity, const Standard_Integer MaxDegree, const Standard_Integer MaxSegments);

Standard_EXPORT Approx_Curve2d(const Handle(Adaptor2d_Curve2d)& C2D,
const Standard_Real First,
const Standard_Real Last,
const Standard_Real TolU,
const Standard_Real TolV,
const GeomAbs_Shape Continuity,
const Standard_Integer MaxDegree,
const Standard_Integer MaxSegments);

Standard_EXPORT Standard_Boolean IsDone() const;

Standard_EXPORT Standard_Boolean HasResult() const;

Standard_EXPORT Handle(Geom2d_BSplineCurve) Curve() const;

Standard_EXPORT Standard_Real MaxError2dU() const;

Standard_EXPORT Standard_Real MaxError2dV() const;

private:

Handle(Geom2d_BSplineCurve) myCurve;
Standard_Boolean myIsDone;
Standard_Boolean myHasResult;
Standard_Real myMaxError2dU;
Standard_Real myMaxError2dV;

Standard_Boolean myIsDone;
Standard_Boolean myHasResult;
Standard_Real myMaxError2dU;
Standard_Real myMaxError2dV;
};

#endif // _Approx_Curve2d_HeaderFile
Loading
Loading