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 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 michicc:gh/smallvec_2 branch 6 times, most recently from 9f34079 to 804f003 Mar 31, 2019
@michicc

This comment has been minimized.

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 michicc:gh/smallvec_2 branch 3 times, most recently from d5b5385 to 6a51ce0 Mar 31, 2019
@michicc

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

M3Henry commented Apr 1, 2019

Oh good, this was on my TODO list

Copy link
Member

LordAro left a comment

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!.

This comment has been minimized.

Copy link
@LordAro

LordAro Apr 2, 2019

Member

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?

This comment has been minimized.

Copy link
@michicc

michicc Apr 2, 2019

Author Member

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?

@michicc michicc force-pushed the michicc:gh/smallvec_2 branch from 6a51ce0 to 572215a Apr 2, 2019
michicc added 9 commits Apr 2, 2019
…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 michicc force-pushed the michicc:gh/smallvec_2 branch from 572215a to 325af5d Apr 7, 2019
@michicc

This comment has been minimized.

Copy link
Member Author

michicc commented Apr 7, 2019

Rebased to drop the already applied OSX commit.

@PeterN

This comment has been minimized.

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

This comment has been minimized.

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.

@LordAro
LordAro approved these changes Apr 9, 2019
@michicc michicc merged commit 8b18801 into OpenTTD:master Apr 9, 2019
8 checks passed
8 checks passed
OpenTTD CI Build #20190407.19 succeeded
Details
OpenTTD CI (Linux commit-checker) Linux commit-checker succeeded
Details
OpenTTD CI (Linux linux-amd64-clang-3.8) Linux linux-amd64-clang-3.8 succeeded
Details
OpenTTD CI (Linux linux-amd64-gcc-6) Linux linux-amd64-gcc-6 succeeded
Details
OpenTTD CI (Linux linux-i386-gcc-6) Linux linux-i386-gcc-6 succeeded
Details
OpenTTD CI (MacOS) MacOS succeeded
Details
OpenTTD CI (Windows Win32) Windows Win32 succeeded
Details
OpenTTD CI (Windows Win64) Windows Win64 succeeded
Details
@michicc michicc deleted the michicc:gh/smallvec_2 branch Apr 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.