refactor: replace C-style casts #3146

AJPfleger · 2024-04-25T14:32:39Z

Summary

This pull request updates to use modern C++ style casts (static_cast<int>(variable), dynamic_cast, reinterpret_cast, const_cast) instead of C-style casts like (int)variable. The change enhances code safety, readability, and maintainability by making type conversions explicit and type-safe.

Replaced most instances of C-style casts with their appropriate C++ style counterparts.
Reviewed each conversion to ensure the chosen C++ cast is suitable for its specific use case.
Use uniform initialization, where it is clear, e.g. type{3.14}

Benefits

Improved Type Safety: Prevents the compiler from performing potentially unsafe conversions.
Increased Clarity and Maintainability: Makes the intentions clear, especially regarding the nature of each cast.
Easier to search for casts by search for _cast<.

CI

I experimented a bit with clang-tidy but couldn't find a proper test, that warns correctly. I tried cppcoreguidelines-pro-type-cstyle-cast but this is only suited for a subset. Using a regex like \(([^\s()]+)\)[a-zA-Z_][a-zA-Z0-9_]* creates too many false positives.

google-readability-casting seems promising and will be added in:

ci: Add clang-tidy check for C-style casts #3149

github-actions · 2024-04-25T15:45:16Z

📊: Physics performance monitoring for `e296442`

Full contents

physmon summary

Examples/Framework/ML/src/NeuralCalibrator.cpp

Examples/Framework/include/ActsExamples/EventData/MuonSimHit.hpp

CouthuresJeremy · 2024-04-26T12:04:08Z

For the CI test, did you try google-readability-casting?

AJPfleger · 2024-04-27T10:29:43Z

For the CI test, did you try google-readability-casting?

@CouthuresJeremy thanks, this one is a bit stricter, than planned, but looks good.

Examples/Io/NuclearInteractions/src/detail/NuclearInteractionParametrisation.cpp

acts-project-service · 2024-04-27T23:56:01Z

✅ Athena integration test results [`eae64a4`]

✅ All tests successful

status	job	report
🟢	run_unit_tests
🟢	test_ActsEFTrackFit
🟢	test_ActsPersistifySeeds
🟢	test_ActsBenchmarkWithSpot
🟢	test_ActsAnalogueClustering
🟢	test_ActsConversionWorkflow
🟢	test_ActsWorkflowHeavyIons
🟢	test_ActsWorkflowFastTracking
🟢	test_ActsWorkflow
🟢	test_ActsValidateAmbiguityResolution
🟢	test_ActsValidateResolvedTracks
🟢	test_ActsValidateTracks
🟢	test_ActsValidateActsCoreSpacePoints
🟢	test_ActsValidateActsSpacePoints
🟢	test_ActsValidateSeeds
🟢	test_ActsValidateOrthogonalSeeds
🟢	test_ActsValidateClusters
🟢	test_ActsPersistifyEDM
🟢	test_ActsGSFRefitting
🟢	test_ActsKfRefitting
🟢	test_ActsExtrapolationAlgTest
🟢	test_ActsITkTest
🟢	run_workflow_tests_run4_mc
🟢	run_workflow_tests_run2_mc
🟢	run_workflow_tests_run2_data
🟢	run_workflow_tests_run3_mc
🟢	run_workflow_tests_run3_data
🟢	run_art_test: test_data18_13TeV_1000evt
🟢	run_art_test: test_ttbarPU40_reco

Adds a new clang-tidy check. Necessary changes and more details in the PR below. blocked by: - #3146 ## Sample ``` ╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Vertexing/SingleSeedVertexF─╮ │ 🟡 │ │ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Vertexing/SingleSeedVertexFi │ │ nderTests.cpp:47:10 WARNING [google-readability-casting] │ │ ╭──────────────────────────────────────────────────────────────────────────╮ │ │ │ C-style casts are discouraged; use static_cast │ │ │ │ 47 | return (int)(gen() / 4294967296. * (to - from) + from); │ │ │ │ | ^~~~~ │ │ │ │ | static_cast<int> │ │ │ │ │ │ │ ╰──────────────────────────────────────────────────────────────────────────╯ │ │ ──────────────────────────────────────────────────────────────────────────── │ ╰──────────────────────────────────────────────────────────────────────────────╯ ╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Material/MaterialCompositio─╮ │ 🟡 │ │ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Material/MaterialComposition │ │ Tests.cpp:28:26 WARNING [google-readability-casting] │ │ ╭──────────────────────────────────────────────────────────────────────────╮ │ │ │ C-style casts are discouraged; use static_cast │ │ │ │ 28 | float carbonFraction = float(carbonWeight) / 255u; │ │ │ │ | ^~~~~ │ │ │ │ | static_cast<float> │ │ │ │ │ │ │ ╰──────────────────────────────────────────────────────────────────────────╯ │ │ ──────────────────────────────────────────────────────────────────────────── │ ╰──────────────────────────────────────────────────────────────────────────────╯ ```

## Summary This pull request updates to use modern C++ style casts (`static_cast<int>(variable)`, `dynamic_cast`, `reinterpret_cast`, `const_cast`) instead of C-style casts like `(int)variable`. The change enhances code safety, readability, and maintainability by making type conversions explicit and type-safe. - Replaced most instances of C-style casts with their appropriate C++ style counterparts. - Reviewed each conversion to ensure the chosen C++ cast is suitable for its specific use case. - Use uniform initialization, where it is clear, e.g. `type{3.14}` ## Benefits - Improved Type Safety: Prevents the compiler from performing potentially unsafe conversions. - Increased Clarity and Maintainability: Makes the intentions clear, especially regarding the nature of each cast. - Easier to search for casts by search for `_cast<`. ## CI I experimented a bit with clang-tidy but couldn't find a proper test, that warns correctly. I tried `cppcoreguidelines-pro-type-cstyle-cast` but this is only suited for a subset. Using a regex like `\(([^\s()]+)\)[a-zA-Z_][a-zA-Z0-9_]*` creates too many false positives. `google-readability-casting` seems promising and will be added in: - acts-project#3149

Adds a new clang-tidy check. Necessary changes and more details in the PR below. blocked by: - acts-project#3146 ## Sample ``` ╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Vertexing/SingleSeedVertexF─╮ │ 🟡 │ │ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Vertexing/SingleSeedVertexFi │ │ nderTests.cpp:47:10 WARNING [google-readability-casting] │ │ ╭──────────────────────────────────────────────────────────────────────────╮ │ │ │ C-style casts are discouraged; use static_cast │ │ │ │ 47 | return (int)(gen() / 4294967296. * (to - from) + from); │ │ │ │ | ^~~~~ │ │ │ │ | static_cast<int> │ │ │ │ │ │ │ ╰──────────────────────────────────────────────────────────────────────────╯ │ │ ──────────────────────────────────────────────────────────────────────────── │ ╰──────────────────────────────────────────────────────────────────────────────╯ ╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Material/MaterialCompositio─╮ │ 🟡 │ │ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Material/MaterialComposition │ │ Tests.cpp:28:26 WARNING [google-readability-casting] │ │ ╭──────────────────────────────────────────────────────────────────────────╮ │ │ │ C-style casts are discouraged; use static_cast │ │ │ │ 28 | float carbonFraction = float(carbonWeight) / 255u; │ │ │ │ | ^~~~~ │ │ │ │ | static_cast<float> │ │ │ │ │ │ │ ╰──────────────────────────────────────────────────────────────────────────╯ │ │ ──────────────────────────────────────────────────────────────────────────── │ ╰──────────────────────────────────────────────────────────────────────────────╯ ```

## Summary This pull request updates to use modern C++ style casts (`static_cast<int>(variable)`, `dynamic_cast`, `reinterpret_cast`, `const_cast`) instead of C-style casts like `(int)variable`. The change enhances code safety, readability, and maintainability by making type conversions explicit and type-safe. - Replaced most instances of C-style casts with their appropriate C++ style counterparts. - Reviewed each conversion to ensure the chosen C++ cast is suitable for its specific use case. - Use uniform initialization, where it is clear, e.g. `type{3.14}` ## Benefits - Improved Type Safety: Prevents the compiler from performing potentially unsafe conversions. - Increased Clarity and Maintainability: Makes the intentions clear, especially regarding the nature of each cast. - Easier to search for casts by search for `_cast<`. ## CI I experimented a bit with clang-tidy but couldn't find a proper test, that warns correctly. I tried `cppcoreguidelines-pro-type-cstyle-cast` but this is only suited for a subset. Using a regex like `\(([^\s()]+)\)[a-zA-Z_][a-zA-Z0-9_]*` creates too many false positives. `google-readability-casting` seems promising and will be added in: - acts-project#3149

Adds a new clang-tidy check. Necessary changes and more details in the PR below. blocked by: - acts-project#3146 ## Sample ``` ╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Vertexing/SingleSeedVertexF─╮ │ 🟡 │ │ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Vertexing/SingleSeedVertexFi │ │ nderTests.cpp:47:10 WARNING [google-readability-casting] │ │ ╭──────────────────────────────────────────────────────────────────────────╮ │ │ │ C-style casts are discouraged; use static_cast │ │ │ │ 47 | return (int)(gen() / 4294967296. * (to - from) + from); │ │ │ │ | ^~~~~ │ │ │ │ | static_cast<int> │ │ │ │ │ │ │ ╰──────────────────────────────────────────────────────────────────────────╯ │ │ ──────────────────────────────────────────────────────────────────────────── │ ╰──────────────────────────────────────────────────────────────────────────────╯ ╭─ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Material/MaterialCompositio─╮ │ 🟡 │ │ /builds/acts/ci-bridge/src/Tests/UnitTests/Core/Material/MaterialComposition │ │ Tests.cpp:28:26 WARNING [google-readability-casting] │ │ ╭──────────────────────────────────────────────────────────────────────────╮ │ │ │ C-style casts are discouraged; use static_cast │ │ │ │ 28 | float carbonFraction = float(carbonWeight) / 255u; │ │ │ │ | ^~~~~ │ │ │ │ | static_cast<float> │ │ │ │ │ │ │ ╰──────────────────────────────────────────────────────────────────────────╯ │ │ ──────────────────────────────────────────────────────────────────────────── │ ╰──────────────────────────────────────────────────────────────────────────────╯ ```

AJPfleger added 6 commits April 25, 2024 14:53

(int)

fff4db4

(float)

9d096b2

(double)

f877d36

rest

522b17d

fix missing ')'

9589d84

format

3b0756f

AJPfleger added this to the next milestone Apr 25, 2024

github-actions bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module Event Data Model Vertexing Track Finding labels Apr 25, 2024

AJPfleger and others added 2 commits April 25, 2024 21:39

Merge branch 'main' into real-casting

9d29a95

M_PI

a24c382

AJPfleger commented Apr 26, 2024

View reviewed changes

Examples/Framework/ML/src/NeuralCalibrator.cpp Show resolved Hide resolved

andiwand previously approved these changes Apr 26, 2024

View reviewed changes

Examples/Framework/include/ActsExamples/EventData/MuonSimHit.hpp Outdated Show resolved Hide resolved

remove cast muonsimhit

578adb7

AJPfleger dismissed andiwand’s stale review via 578adb7 April 26, 2024 11:46

from new check

a5121c1

github-actions bot added the Component - Plugins Affects one or more Plugins label Apr 26, 2024

AJPfleger added 4 commits April 26, 2024 16:30

revert transform3

534f13d

revert transform3 2

0a02167

almost everything

31e783a

try google-readability-casting

4ea890a

github-actions bot added Infrastructure Changes to build tools, continous integration, ... Seeding labels Apr 26, 2024

test uniform initialisation

302463f

AJPfleger and others added 7 commits April 27, 2024 09:04

narrowing conversion

362f468

fix throw message

1b8432b

fix missing }

e835110

format [skip ci]

7269f06

Merge branch 'main' into real-casting

44226d4

final changes? [skip ci]

c1a625d

revert clang-tidy

b8c5fb5

github-actions bot removed the Infrastructure Changes to build tools, continous integration, ... label Apr 27, 2024

AJPfleger removed the 🚧 WIP Work-in-progress label Apr 27, 2024

AJPfleger requested a review from andiwand April 27, 2024 11:08

andiwand previously approved these changes Apr 27, 2024

View reviewed changes

Examples/Io/NuclearInteractions/src/detail/NuclearInteractionParametrisation.cpp Outdated Show resolved Hide resolved

try non-float counter

814a661

AJPfleger dismissed andiwand’s stale review via 814a661 April 27, 2024 18:25

andiwand previously approved these changes Apr 27, 2024

View reviewed changes

Examples/Io/NuclearInteractions/src/detail/NuclearInteractionParametrisation.cpp Outdated Show resolved Hide resolved

Examples/Io/NuclearInteractions/src/detail/NuclearInteractionParametrisation.cpp Outdated Show resolved Hide resolved

remove unnecessary casts

e296442

AJPfleger dismissed andiwand’s stale review via e296442 April 27, 2024 20:20

andiwand approved these changes Apr 27, 2024

View reviewed changes

AJPfleger added the automerge label Apr 27, 2024

kodiakhq bot merged commit eae64a4 into acts-project:main Apr 27, 2024
53 checks passed

github-actions bot removed the automerge label Apr 27, 2024

AJPfleger deleted the real-casting branch April 27, 2024 21:39

andiwand modified the milestones: next, v35.0.0 May 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace C-style casts #3146

refactor: replace C-style casts #3146

AJPfleger commented Apr 25, 2024 •

edited

Loading

github-actions bot commented Apr 25, 2024 •

edited

Loading

CouthuresJeremy commented Apr 26, 2024

AJPfleger commented Apr 27, 2024

acts-project-service commented Apr 27, 2024

refactor: replace C-style casts #3146

refactor: replace C-style casts #3146

Conversation

AJPfleger commented Apr 25, 2024 • edited Loading

Summary

Benefits

CI

github-actions bot commented Apr 25, 2024 • edited Loading

📊: Physics performance monitoring for e296442

physmon summary

CouthuresJeremy commented Apr 26, 2024

AJPfleger commented Apr 27, 2024

acts-project-service commented Apr 27, 2024

✅ Athena integration test results [eae64a4]

✅ All tests successful

AJPfleger commented Apr 25, 2024 •

edited

Loading

github-actions bot commented Apr 25, 2024 •

edited

Loading

📊: Physics performance monitoring for `e296442`

✅ Athena integration test results [`eae64a4`]