[core][chore] Use make_shared to create shared ptr if possible #49430
Conversation
kevin85421
changed the title
Signed-off-by: kaihsun <[email protected]>
Signed-off-by: kaihsun <[email protected]>
Signed-off-by: kaihsun <[email protected]>
There was a problem hiding this comment.
clang-tidy actually auto-fixes this too, been going through and using to lint the codebase, https://clang.llvm.org/extra/clang-tidy/checks/modernize/make-shared.html
| @@ -25,6 +25,8 @@ class DefaultCoreWorkerMemoryStoreWithThread : public CoreWorkerMemoryStore { | |||
| std::unique_ptr<InstrumentedIOContextWithThread> io_context = | |||
| std::make_unique<InstrumentedIOContextWithThread>( | |||
| "DefaultCoreWorkerMemoryStoreWithThread"); | |||
| // C++ limitation: std::make_unique cannot be used because std::unique_ptr cannot | |||
| // invoke private constructors. | |||
| return std::unique_ptr<DefaultCoreWorkerMemoryStoreWithThread>( | |||
| new DefaultCoreWorkerMemoryStoreWithThread(std::move(io_context))); | |||
There was a problem hiding this comment.
imo not as necessary to comment for make_unique, same perf benefit doesn't apply because there's no separate allocation for ref counting, but still good to be consistent
There was a problem hiding this comment.
Based on this YouTube video, make_unique is still better compared to explicitly calling the constructor due to its exception safety.
There was a problem hiding this comment.
Good to know! This should be a better option. Still learning C++ 🙂 |
Why are these changes needed?
This YouTube video mentions that
make_sharedis better thanstd::shared_ptr<MyClass>(new MyClass(...))in two aspects:make_sharedrequires only one memory allocation.make_sharedis exception-safe.I quickly went through all lines of code matching
std::shared_ptr<in the repo.make_sharedif the constructor is accessible.Related issue number
Checks
git commit -s) in this PR.scripts/format.shto lint the changes in this PR.method in Tune, I've added it in
doc/source/tune/api/under thecorresponding
.rstfile.