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

Use std::make_shared to allocate shared ptrs, instead of std::shared_ptr constructors #556

Merged
merged 7 commits into from Oct 16, 2020

Conversation

ferdnyc
Copy link
Contributor

@ferdnyc ferdnyc commented Aug 20, 2020

std::make_shared does in a single allocation what the constructors for std::shared_ptr usually take at least 2 allocations to do.

//
// This PR replaces instances of shared-pointer allocations like:
//
std::shared_ptr<Type> name = std::shared_ptr<Type>(new Type(...));
name = std::shared_ptr<Type>(new Type(...));
std::shared_ptr<Type> name(new Type(...));
//
// with calls to the make_shared factory function, e.g:
//
auto name = std::make_shared<Type>(...);
name = std::make_shared<Type>(...);

As you can see, the revised syntax is at worst as concise, and in some cases much more concise, than the previous code. And, doing this may give us an infinitesimal performance/memory improvement (though I don't expect it's measurable except in contrived benchmarks).

For said contrived benchmarks, see: https://www.modernescpp.com/index.php/memory-and-performance-overhead-of-smart-pointer

std::make_shared does in a single allocation what the constructors
for std::shared_ptr usually take at least 2 allocations to do.
May give us an infinitesimal performance/memory improvement.

https://www.modernescpp.com/index.php/memory-and-performance-overhead-of-smart-pointer
@codecov
Copy link

codecov bot commented Aug 20, 2020

Codecov Report

Merging #556 into develop will increase coverage by 0.56%.
The diff coverage is 53.52%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #556      +/-   ##
===========================================
+ Coverage    48.77%   49.33%   +0.56%     
===========================================
  Files          129      129              
  Lines        10036    10207     +171     
===========================================
+ Hits          4895     5036     +141     
- Misses        5141     5171      +30     
Impacted Files Coverage Δ
include/DummyReader.h 0.00% <ø> (ø)
include/Frame.h 100.00% <ø> (ø)
include/effects/Pixelate.h 0.00% <ø> (ø)
src/ChunkWriter.cpp 0.00% <0.00%> (ø)
src/Qt/VideoRenderWidget.cpp 0.00% <ø> (ø)
src/QtHtmlReader.cpp 0.00% <0.00%> (ø)
src/QtTextReader.cpp 0.00% <0.00%> (ø)
src/TextReader.cpp 0.00% <0.00%> (ø)
src/effects/Bars.cpp 0.00% <0.00%> (ø)
src/effects/Crop.cpp 0.00% <0.00%> (ø)
... and 26 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d121f9d...0bcf1e4. Read the comment docs.

Best reason not to narrate the code in the comments: The code gets
changed, but the documentation doesn't.
@jonoomph
Copy link
Member

LGTM!

@ferdnyc
Copy link
Contributor Author

ferdnyc commented Sep 13, 2020

@jonoomph I decided to let Codacy shame me into fixing that initialization issue, and replace all of the redundant Frame constructors with delegating constructors.

@jonoomph
Copy link
Member

This one is going to have some conflicts with my new clip-refactor-keyframes branch, yikes.

@jonoomph
Copy link
Member

This might wait until after I merge clip-refactor-keyframes branch

@jonoomph
Copy link
Member

Okay, I'm going to bite the bullet, merge this, and then merge develop back into my clip-refactor-keyframes branch and resolve all the errors.

@jonoomph jonoomph merged commit 8f6c642 into OpenShot:develop Oct 16, 2020
@ferdnyc ferdnyc deleted the use-make-shared branch May 4, 2021 23:25
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.

None yet

2 participants