experimental: use clang-format for buffer and graph by barendgehrels · Pull Request #1400 · boostorg/geometry

barendgehrels · 2025-04-26T10:51:27Z

Applied on two folders only!

This is experimental and verifies how clang-format could be used for our code base,
using a style that is most close to what we (in general) use.

I used clang-format-15.

Other Boost libraries

clang-format is used by a few other boost libraries. I tried some of these configurations, but it was not satisfactory to my taste.
Here an indication (on a branch of somewhere last year), with sizes in bytes:

  414  ./graph/.clang-format
  706  ./multiprecision/.clang-format
 1721  ./outcome/.clang-format
 2561  ./histogram/.clang-format
 3111  ./yap/.clang-format
 4108  ./nowide/.clang-format
 5697  ./locale/.clang-format

  685 ./geometry/.clang-format

barendgehrels · 2025-04-26T10:52:14Z

include/boost/geometry/algorithms/detail/buffer/buffer_box.hpp

-namespace boost { namespace geometry
+namespace boost
+{
+namespace geometry


I cannot find a rule which doesn't affect this style for namespace.
Maybe we can live with this change.

With the more detailled Brace rules as in #1400 (comment) , I can get namespace boost { namespace geometry {

Yes, better for sure! Thanks! Applied!

barendgehrels · 2025-04-26T10:52:55Z

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

    using turn_vector_type = std::vector<buffer_turn_info_type>;

-    using piece_border_type = piece_border<Ring, point_type> ;
+    using piece_border_type = piece_border<Ring, point_type>;


It nicely catches these kind of small errors.

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp

barendgehrels · 2025-04-26T10:56:15Z

include/boost/geometry/algorithms/detail/buffer/turn_in_piece_visitor.hpp

+          typename Turns,
+          typename Pieces,
+          typename DistanceStrategy,
+          typename UmbrellaStrategy


This is also different than what we had. But it is (as far as I can see) the variant closest possible.

barendgehrels · 2025-04-26T10:57:28Z

include/boost/geometry/algorithms/detail/overlay/graph/get_tois.hpp

+        auto const source_turn_indices
+            = get_turn_indices_by_node_id(turns, clusters, source_node_id, allow_closed);
+        auto const target_turn_indices
+            = get_turn_indices_by_node_id(turns, clusters, target_node_id, allow_closed);


Nice catches of wrong const placements, from me recently...

barendgehrels · 2025-04-26T10:57:58Z

include/boost/geometry/algorithms/detail/overlay/graph/get_tois.hpp

-            << std::endl;
+        std::cout << "quadratic: " << source_node_id << " -> " << target_node_id << " sizes "
+                  << source_turn_indices.size() << " x " << target_turn_indices.size() << " = "
+                  << result.size() << std::endl;


This is not ideal IMO but I don't think we can get this completely satisfactory

barendgehrels · 2025-04-26T10:59:06Z

include/boost/geometry/algorithms/detail/overlay/graph/is_target_operation.hpp

-struct is_better_collinear_target {};
+struct is_better_collinear_target
+{
+};


This is somehow split (though I have AllowShortBlocksOnASingleLine)

With the more detailled Brace rules as in #1400 (comment) , I can get

template <operation_type Operation> struct is_better_collinear_target {};

include/boost/geometry/algorithms/detail/overlay/graph/select_edge.hpp

barendgehrels · 2025-04-26T11:07:34Z

include/boost/geometry/algorithms/detail/overlay/graph/select_edge.hpp

-                bool const better = is_better_collinear_for_union(
-                        op0, op1, edges.front().toi, edges.back().toi);
+                bool const better
+                    = is_better_collinear_for_union(op0, op1, edges.front().toi, edges.back().toi);


This is nicer IMO

barendgehrels · 2025-04-26T11:10:01Z

include/boost/geometry/algorithms/detail/buffer/buffer_inserter.hpp

+        return side == -1                   ? geometry::strategy::buffer::join_convex
+               : side == 1                  ? geometry::strategy::buffer::join_concave
+               : same_direction(p0, p1, p2) ? geometry::strategy::buffer::join_continue
+                                            : geometry::strategy::buffer::join_spike;


This is not too bad, actually better than I've ever seen from clang-format

barendgehrels · 2025-04-26T11:11:43Z

include/boost/geometry/algorithms/detail/buffer/buffer_inserter.hpp

+                                                                      first_p1,
+                                                                      first_p2,
+                                                                      last_p1,
+                                                                      last_p2);


not really nice... but still acceptable probably

tinko92 · 2025-04-27T06:39:26Z

This looks very convenient and I think having a style that can be programmatically enforced but is slightly different from before is better than having a style that is more difficult to ensure consistency for, I'm definitely in favour of this change.

The behaviour for namespaces and empty structs can maybe be made closer to the original by breaking up the Allman style into custom rules to partially address #1400 (comment) and #1400 (comment) like this:

-BreakBeforeBraces: Allman
+BreakBeforeBraces: Custom
+BraceWrapping:
+  AfterClass: true
+  AfterControlStatement: true
+  AfterEnum: true
+  AfterFunction: true
+  AfterNamespace: false
+  AfterStruct: true
+  AfterUnion: true
+  AfterExternBlock: true
+  BeforeCatch: true
+  BeforeElse: true
+  BeforeLambdaBody: true
+  SplitEmptyFunction: false
+  SplitEmptyRecord: false
+  SplitEmptyNamespace: false
+  IndentBraces: false

The following may also be of interest, if this is to be mass-applied, to somewhat preserve the ability to use git blame to understand why some particular line was changed, it requires logging the commit in a file called .git-blame-ignore-revs at root.
https://docs.github.com/en/repositories/working-with-files/using-files/viewing-and-understanding-files#ignore-commits-in-the-blame-view

barendgehrels · 2025-05-15T17:02:44Z

hi @vissarion , I'll not merge it - but just to gauge your opinion, do you like it and the proposed styling?

barendgehrels · 2025-05-15T17:04:55Z

The following may also be of interest, if this is to be mass-applied, to somewhat preserve the ability to use git blame to understand why some particular line was changed, it requires logging the commit in a file called .git-blame-ignore-revs at root.
https://docs.github.com/en/repositories/working-with-files/using-files/viewing-and-understanding-files#ignore-commits-in-the-blame-view

Very interesting, thanks! So we should use that indeed.

vissarion · 2025-05-16T09:44:52Z

hi @vissarion , I'll not merge it - but just to gauge your opinion, do you like it and the proposed styling?

I definitely like it and I am in favor of using clang-format.

experimental: use clang-format for buffer and graph

9760179

barendgehrels requested review from awulkiew, tinko92 and vissarion April 26, 2025 10:51

barendgehrels commented Apr 26, 2025

View reviewed changes

include/boost/geometry/algorithms/detail/buffer/buffered_piece_collection.hpp Outdated Show resolved Hide resolved

barendgehrels commented Apr 26, 2025

View reviewed changes

fix access modifier

6aabaaf

barendgehrels commented Apr 26, 2025

View reviewed changes

include/boost/geometry/algorithms/detail/overlay/graph/select_edge.hpp Outdated Show resolved Hide resolved

barendgehrels commented Apr 26, 2025

View reviewed changes

fix BreakConstructorInitializers

4ef47a9

barendgehrels self-assigned this Apr 26, 2025

barendgehrels added 2 commits April 27, 2025 10:50

apply Tinko's suggestion

edc7c52

add space before case colon

1db5765

Conversation

barendgehrels commented Apr 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Applied on two folders only!

Other Boost libraries

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tinko92 commented Apr 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

barendgehrels commented May 15, 2025

Uh oh!

barendgehrels commented May 15, 2025

Uh oh!

vissarion commented May 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

barendgehrels commented Apr 26, 2025 •

edited

Loading

tinko92 commented Apr 27, 2025 •

edited

Loading