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

Remove AutoDeleteSmallVector and AutoFreeSmallVector #7453

Merged
merged 9 commits into from Apr 9, 2019

Conversation

michicc
Copy link
Member

@michicc michicc commented Mar 31, 2019

Remove the last remains of SmallVector. For the replacement, std::unique_ptr takes care of the memory management. Use of references and move semantics makes ownership clear and reduces potentials for memory leaks. Current compilers should be able to optimize most of the copy/moves out, resulting in very little runtime overhead.

This got a bit bigger than expected and picked up some unrelated fixes I saw when in the area.

Commits 41b6916, 0266dcb, and b27d0af could be split out if needed.

@michicc michicc force-pushed the gh/smallvec_2 branch 6 times, most recently from 9f34079 to 804f003 Compare March 31, 2019 23:11
@michicc
Copy link
Member Author

michicc commented Mar 31, 2019

There's a slight work-around for clang-3.8 included (extra std::move that shouldn't be needed). Starting with clang-3.9 (verified with godbolt) this wouldn't be needed anymore.

@michicc michicc force-pushed the gh/smallvec_2 branch 3 times, most recently from d5b5385 to 6a51ce0 Compare April 1, 2019 20:10
@michicc
Copy link
Member Author

michicc commented Apr 1, 2019

Now with more std::string to please @nielsmh 😁

Also, why the heck is GitHub displaying the commits in a mostly random order?

@M3Henry
Copy link
Contributor

M3Henry commented Apr 1, 2019

Oh good, this was on my TODO list

Copy link
Member

@LordAro LordAro left a comment

Choose a reason for hiding this comment

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

use of noexcept is interesting. I've seen varying opinions on how worthwhile it actually is...

src/widgets/dropdown.cpp Show resolved Hide resolved
src/gfx_layout.cpp Outdated Show resolved Hide resolved
AutoDeleteSmallVector<LanguageStrings *> compiled_strings; ///< The compiled strings per language, first must be English/the master language!.
StringList string_names; ///< The names of the compiled strings.
std::vector<std::unique_ptr<LanguageStrings>> raw_strings; ///< The raw strings per language, first must be English/the master language!.
std::vector<std::shared_ptr<LanguageStrings>> compiled_strings; ///< The compiled strings per language, first must be English/the master language!.
Copy link
Member

Choose a reason for hiding this comment

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

how difficult would it be to "fix" the use of shared_ptr here?
or, perhaps the various places that "own" the pointer could be documented?

Copy link
Member Author

Choose a reason for hiding this comment

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

Very easily, it is only GameStrings::cur_language that points to one of the compiled string entries. We store a raw pointer now and just assume it's never going to be invalidated; still possible.

In pure though, raw pointers are bad and should never be used. Of course, the codebase is far, far, far from that point now. So, philosophical question time: do the "right" thing, or the pragmatic thing?

…ror.

The raw_strings vector may not include NULLs as no consumer can deal with it.
…r instead of heap allocated.

This reduces memory allocations and heap fragmentation.
…ing AutoDeleteSmallVector obsolete.

DropDownListItem are strongly managed using std::unique_ptr to ensure leak-free handling. Appropriate use
of move-semantics make intent a lot clearer than parameter comments and allows the compiler to generate
copy-free code for most situations.
The last use was for storing a list of memory blocks. As the way these lists are accessed is very
specific, it is easier to just write an explicit destructor instead of trying to exactly match the behaviour.
@michicc
Copy link
Member Author

michicc commented Apr 7, 2019

Rebased to drop the already applied OSX commit.

@PeterN
Copy link
Member

PeterN commented Apr 7, 2019

Does the change from returning a DropDownList * to returning a DropDownList mean it makes a copy?

@michicc
Copy link
Member Author

michicc commented Apr 8, 2019

In the literal sense: No. std::vector is a moveable type, which means the return statement will resolve to a move assignment and not a copy assignment. Moving a vector is quite cheap.

In the less literal sense: Many current compilers will perform copy elision where possible, which means even the move assignment is likely to be optimized out.

@michicc michicc merged commit 8b18801 into OpenTTD:master Apr 9, 2019
@michicc michicc deleted the gh/smallvec_2 branch April 9, 2019 20:45
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

4 participants