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

ci: Add clang-tidy check for nested namespace definitions #3084

Merged
merged 15 commits into from
Apr 8, 2024

Conversation

AJPfleger
Copy link
Contributor

@AJPfleger AJPfleger commented Apr 5, 2024

New language feature in C++17:
Nested namespace definitions, e.g., namespace X::Y { … } instead of namespace X { namespace Y { … } }

We had a good mix between them, but clang-tidy has with modernize-concat-nested-namespaces an option, to automatically check for them.

I stumbled over this, when reading through the cpp-standard-updates and I think this improves readability.

Hits will be reported similar to this one:

╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Plugins/TGeo/TGeoParserTests.cpp─╮
│ 🟡                                                                           │
│ /builds/acts/ci-bridge/src/Tests/UnitTests/Plugins/TGeo/TGeoParserTests.cpp: │
│ 27:1 WARNING [modernize-concat-nested-namespaces]                            │
│ ╭──────────────────────────────────────────────────────────────────────────╮ │
│ │ nested namespaces can be concatenated                                    │ │
│ │    27 | namespace Acts {                                                 │ │
│ │       | ^~~~~~~~~~~~~~~~                                                 │ │
│ │    28 |                                                                  │ │
│ │    29 | namespace Test {                                                 │ │
│ │       | ~~~~~~~~~~~~~~                                                   │ │
│ │       | namespace Acts::Test                                             │ │
│ │                                                                          │ │
│ ╰──────────────────────────────────────────────────────────────────────────╯ │
│ ──────────────────────────────────────────────────────────────────────────── │
╰──────────────────────────────────────────────────────────────────────────────╯

@AJPfleger AJPfleger added this to the next milestone Apr 5, 2024
@AJPfleger AJPfleger added the 🚧 WIP Work-in-progress label Apr 5, 2024
Copy link

codecov bot commented Apr 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.02%. Comparing base (1374baa) to head (ddcfd7f).

❗ Current head ddcfd7f differs from pull request most recent head 99f9ce8. Consider uploading reports for the commit 99f9ce8 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3084   +/-   ##
=======================================
  Coverage   49.02%   49.02%           
=======================================
  Files         494      494           
  Lines       29058    29058           
  Branches    13797    13797           
=======================================
  Hits        14246    14246           
  Misses       4929     4929           
  Partials     9883     9883           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Apr 6, 2024
@github-actions github-actions bot removed the Infrastructure Changes to build tools, continous integration, ... label Apr 6, 2024
@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Apr 6, 2024
@github-actions github-actions bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Component - Plugins Affects one or more Plugins Event Data Model Clustering Vertexing Track Finding labels Apr 6, 2024
@AJPfleger AJPfleger removed the 🚧 WIP Work-in-progress label Apr 7, 2024
Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

👍

I took a quick look at all files. No objections from my side.

I think the nested namespace syntax is more compact.

@kodiakhq kodiakhq bot merged commit 6883f31 into acts-project:main Apr 8, 2024
55 checks passed
@github-actions github-actions bot removed the automerge label Apr 8, 2024
@acts-project-service acts-project-service added Breaks Athena build This PR breaks the Athena build Fails Athena tests This PR causes a failure in the Athena tests labels Apr 8, 2024
@AJPfleger AJPfleger deleted the namespaces branch April 9, 2024 13:22
@andiwand andiwand removed Breaks Athena build This PR breaks the Athena build Fails Athena tests This PR causes a failure in the Athena tests labels Apr 10, 2024
@andiwand andiwand modified the milestones: next, v34.0.0 Apr 15, 2024
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Apr 19, 2024
…ct#3084)

New language feature in C++17:
Nested namespace definitions, e.g., `namespace X::Y { … }` instead of `namespace X { namespace Y { … } }`

We had a good mix between them, but clang-tidy has with `modernize-concat-nested-namespaces` an option, to automatically check for them.

I stumbled over this, when reading through the cpp-standard-updates and I think this improves readability.

Hits will be reported similar to this one:
```
╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Plugins/TGeo/TGeoParserTests.cpp─╮
│ 🟡                                                                           │
│ /builds/acts/ci-bridge/src/Tests/UnitTests/Plugins/TGeo/TGeoParserTests.cpp: │
│ 27:1 WARNING [modernize-concat-nested-namespaces]                            │
│ ╭──────────────────────────────────────────────────────────────────────────╮ │
│ │ nested namespaces can be concatenated                                    │ │
│ │    27 | namespace Acts {                                                 │ │
│ │       | ^~~~~~~~~~~~~~~~                                                 │ │
│ │    28 |                                                                  │ │
│ │    29 | namespace Test {                                                 │ │
│ │       | ~~~~~~~~~~~~~~                                                   │ │
│ │       | namespace Acts::Test                                             │ │
│ │                                                                          │ │
│ ╰──────────────────────────────────────────────────────────────────────────╯ │
│ ──────────────────────────────────────────────────────────────────────────── │
╰──────────────────────────────────────────────────────────────────────────────╯
```
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
…ct#3084)

New language feature in C++17:
Nested namespace definitions, e.g., `namespace X::Y { … }` instead of `namespace X { namespace Y { … } }`

We had a good mix between them, but clang-tidy has with `modernize-concat-nested-namespaces` an option, to automatically check for them.

I stumbled over this, when reading through the cpp-standard-updates and I think this improves readability.

Hits will be reported similar to this one:
```
╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Plugins/TGeo/TGeoParserTests.cpp─╮
│ 🟡                                                                           │
│ /builds/acts/ci-bridge/src/Tests/UnitTests/Plugins/TGeo/TGeoParserTests.cpp: │
│ 27:1 WARNING [modernize-concat-nested-namespaces]                            │
│ ╭──────────────────────────────────────────────────────────────────────────╮ │
│ │ nested namespaces can be concatenated                                    │ │
│ │    27 | namespace Acts {                                                 │ │
│ │       | ^~~~~~~~~~~~~~~~                                                 │ │
│ │    28 |                                                                  │ │
│ │    29 | namespace Test {                                                 │ │
│ │       | ~~~~~~~~~~~~~~                                                   │ │
│ │       | namespace Acts::Test                                             │ │
│ │                                                                          │ │
│ ╰──────────────────────────────────────────────────────────────────────────╯ │
│ ──────────────────────────────────────────────────────────────────────────── │
╰──────────────────────────────────────────────────────────────────────────────╯
```
asalzburger pushed a commit to asalzburger/acts that referenced this pull request May 21, 2024
…ct#3084)

New language feature in C++17:
Nested namespace definitions, e.g., `namespace X::Y { … }` instead of `namespace X { namespace Y { … } }`

We had a good mix between them, but clang-tidy has with `modernize-concat-nested-namespaces` an option, to automatically check for them.

I stumbled over this, when reading through the cpp-standard-updates and I think this improves readability.

Hits will be reported similar to this one:
```
╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Plugins/TGeo/TGeoParserTests.cpp─╮
│ 🟡                                                                           │
│ /builds/acts/ci-bridge/src/Tests/UnitTests/Plugins/TGeo/TGeoParserTests.cpp: │
│ 27:1 WARNING [modernize-concat-nested-namespaces]                            │
│ ╭──────────────────────────────────────────────────────────────────────────╮ │
│ │ nested namespaces can be concatenated                                    │ │
│ │    27 | namespace Acts {                                                 │ │
│ │       | ^~~~~~~~~~~~~~~~                                                 │ │
│ │    28 |                                                                  │ │
│ │    29 | namespace Test {                                                 │ │
│ │       | ~~~~~~~~~~~~~~                                                   │ │
│ │       | namespace Acts::Test                                             │ │
│ │                                                                          │ │
│ ╰──────────────────────────────────────────────────────────────────────────╯ │
│ ──────────────────────────────────────────────────────────────────────────── │
╰──────────────────────────────────────────────────────────────────────────────╯
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clustering Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Component - Plugins Affects one or more Plugins Event Data Model Infrastructure Changes to build tools, continous integration, ... Seeding SP formation Track Finding Track Fitting Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants