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

[Core] Remove unnecessary new operators ... #1313

Merged
merged 4 commits into from
Jan 22, 2024
Merged

Conversation

KerstinKeller
Copy link
Contributor

@KerstinKeller KerstinKeller commented Jan 18, 2024

and replace with make_shared or make_unique.

Description

There is really need for explicit use of new so replace with creation of shared/unique pointers.

Cherry-pick to:

  • None (changes public API slightly and will therefore be ABI compatible)

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

ecal/core/include/ecal/ecal_timer.h Show resolved Hide resolved
ecal/core/include/ecal/ecal_timer.h Show resolved Hide resolved

/**
* @brief Query if this object is created.
*
* @return true if created, false if not.
**/
bool IsCreated() { return(created); }
ECAL_API bool IsCreated() { return(created); }
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: method 'IsCreated' can be made const [readability-make-member-function-const]

Suggested change
ECAL_API bool IsCreated() { return(created); }
ECAL_API bool IsCreated() const { return(created); }

- Respect rule of 5
@FlorianReimold FlorianReimold self-assigned this Jan 19, 2024
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

ecal/core/include/ecal/ecal_timer.h Outdated Show resolved Hide resolved
/**
* @brief Move assignment
**/
ECAL_API CTimer& operator=(CTimer&& rhs);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]

Suggested change
ECAL_API CTimer& operator=(CTimer&& rhs);
ECAL_API CTimer& operator=(CTimer&& rhs) noexcept ;

/**
* @brief Move assignment
**/
ECAL_API CDynamicJSONSubscriber& operator=(CDynamicJSONSubscriber&& rhs);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]

Suggested change
ECAL_API CDynamicJSONSubscriber& operator=(CDynamicJSONSubscriber&& rhs);
ECAL_API CDynamicJSONSubscriber& operator=(CDynamicJSONSubscriber&& rhs) noexcept ;

ecal/core/src/pubsub/ecal_proto_dyn_json_sub.cpp Outdated Show resolved Hide resolved
CDynamicJSONSubscriber::CDynamicJSONSubscriber(CDynamicJSONSubscriber&& rhs)
= default;

CDynamicJSONSubscriber& CDynamicJSONSubscriber::operator=(CDynamicJSONSubscriber&& rhs)
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: move assignment operators should be marked noexcept [performance-noexcept-move-constructor]

ecal/core/src/pubsub/ecal_proto_dyn_json_sub.cpp:215:

-       = default;
+  noexcept       = default;

ecal/core/src/time/ecal_timer.cpp Outdated Show resolved Hide resolved
ecal/core/src/time/ecal_timer.cpp Outdated Show resolved Hide resolved
@KerstinKeller
Copy link
Contributor Author

We should rather delete the move operator than to just default them, it might lead to problems.

@KerstinKeller
Copy link
Contributor Author

Ubuntu build fails with:

5: Test command: /home/runner/work/ecal/_build/bin/ecal_test_core-5.13.0
5: Working Directory: /home/runner/work/ecal/_build/bin
5: Test timeout computed to be: 10000000
5: Running main() from /home/runner/work/ecal/ecal/thirdparty/gtest/googletest/googletest/src/gtest_main.cc
5: [==========] Running 3 tests from 1 test suite.
5: [----------] Global test environment set-up.
5: [----------] 3 tests from Core
5: [ RUN      ] Core.MultipleInitializeFinalize
5: [       OK ] Core.MultipleInitializeFinalize (7006 ms)
5: [ RUN      ] Core.LeakedPubSub
5: [       OK ] Core.LeakedPubSub (5004 ms)
5: [ RUN      ] Core.CallbackDestruction
5: double free or corruption (out)
 5/14 Test  #5: test_core ........................Subprocess aborted***Exception:  16.16 sec

Need to check if sporadic failure (even though - why?) or reproducible from the changes (though I wouldn't know / understand why...).

@KerstinKeller KerstinKeller merged commit 8c0be55 into master Jan 22, 2024
14 checks passed
@KerstinKeller KerstinKeller deleted the feature/no-raw-new branch January 25, 2024 18:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants