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
PR for Modernizing for C++11: replacing instances of SmallVector #6817
Changes from all commits
6447f50
e397df9
ec544d7
cd8d6cc
ccf3c5c
fbc5e75
e894abe
daa7997
2561105
c95b308
bee3249
8897223
7845eed
573b1b2
27c0082
72d73e3
445bcdb
f748fcd
633629c
40dd391
81b7b07
d6fba11
054063e
2f119f1
43b6222
e660300
9804906
d4d0b5d
1f3845a
072d1b9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -7,14 +7,131 @@ | |||||||
* See the GNU General Public License for more details. You should have received a copy of the GNU General Public License along with OpenTTD. If not, see <http://www.gnu.org/licenses/>. | ||||||||
*/ | ||||||||
|
||||||||
/** @file smallvec_type.hpp Simple vector class that allows allocating an item without the need to copy this->data needlessly. */ | ||||||||
/** @file smallvec_type.hpp Simple vector class that allows allocating an item without the need to copy this->data needlessly. | ||||||||
* @note The std::vector adapter functions are _temporary_ substitutions for methods of SmallVector | ||||||||
*/ | ||||||||
|
||||||||
#ifndef SMALLVEC_TYPE_HPP | ||||||||
#define SMALLVEC_TYPE_HPP | ||||||||
|
||||||||
#include "alloc_func.hpp" | ||||||||
#include "mem_func.hpp" | ||||||||
|
||||||||
#include <vector> | ||||||||
#include <algorithm> | ||||||||
|
||||||||
/** Get a const iterator to a matching element in the vector. | ||||||||
* @param vec the vector to be searched | ||||||||
* @param elem the element to search fo | ||||||||
* @tparam T the contained type of the vector | ||||||||
*/ | ||||||||
template <typename T> | ||||||||
typename std::vector<T>::const_iterator Find(const std::vector<T>& vec, const T& elem) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. opening brace of a function should be on a newline (applies to various functions) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
return std::find(vec.begin(), vec.end(), elem); | ||||||||
} | ||||||||
|
||||||||
/** Get a non-const iterator to a matching element in the vector. | ||||||||
* @param vec the vector to be searched | ||||||||
* @param elem the element to search for | ||||||||
* @tparam T the contained type of the vector | ||||||||
*/ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missing (applies to various functions) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
template <typename T> | ||||||||
typename std::vector<T>::iterator Find(std::vector<T>& vec, const T& elem) { | ||||||||
return std::find(vec.begin(), vec.end(), elem); | ||||||||
} | ||||||||
|
||||||||
/** Get the index of the first matching element. | ||||||||
* @param vec the vector to be searched | ||||||||
* @param elem the element to search for | ||||||||
* @tparam T the contained type of the vector | ||||||||
*/ | ||||||||
template <typename T> | ||||||||
int FindIndex(const std::vector<T>& vec, const T& elem) { | ||||||||
auto it = Find(vec, elem); | ||||||||
if (vec.end() == it) return -1; | ||||||||
return std::distance(vec.begin(), it); | ||||||||
} | ||||||||
|
||||||||
/** Check whether an element exists. | ||||||||
* @param vec the vector to be searched | ||||||||
* @param elem the element to search for | ||||||||
* @tparam T the contained type of the vector | ||||||||
*/ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
template <typename T> | ||||||||
bool Contains(const std::vector<T>& vec, const T& elem) { | ||||||||
return Find(vec, elem) != vec.end(); | ||||||||
} | ||||||||
|
||||||||
/** Count instances of an element. | ||||||||
* @param vec the vector to be searched | ||||||||
* @param elem the element to count instances of | ||||||||
* @tparam T the contained type of the vector | ||||||||
*/ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
template <typename T> | ||||||||
size_t Count(const std::vector<T>& vec, const T& elem) { | ||||||||
return std::count(vec.begin(), vec.end(), elem); | ||||||||
} | ||||||||
|
||||||||
/** Append an element if it does not already exist in the vector. | ||||||||
* @param vec the vector to be modified | ||||||||
* @param elem the element to add for and remove if missing | ||||||||
* @tparam T the contained type of the vector | ||||||||
* @tparam U the universal reference type for perfect forwarding | ||||||||
*/ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
template <typename T, typename U> | ||||||||
bool Include(std::vector<T>& vec, U&& elem) { | ||||||||
auto missing = not Contains(vec, elem); | ||||||||
if (missing) vec.push_back(std::forward<U>(elem)); | ||||||||
return missing; | ||||||||
} | ||||||||
|
||||||||
/** Remove an element referenced in the vector and move the last element to take its place. | ||||||||
* @param vec the vector to be modified | ||||||||
* @param it the element to remove | ||||||||
* @tparam T the contained type of the vector | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
*/ | ||||||||
template <typename T> | ||||||||
typename std::vector<T>::iterator Erase(std::vector<T>& vec, typename std::vector<T>::iterator it) { | ||||||||
assert(vec.begin() <= it and vec.end() > it); | ||||||||
if (std::prev(vec.end()) != it) std::swap(vec.back(), *it); | ||||||||
vec.pop_back(); | ||||||||
return it; | ||||||||
} | ||||||||
|
||||||||
/** Remove an element if it exists in the vector and move the last element to take its place. | ||||||||
* @param vec the vector to be modified | ||||||||
* @param elem the element to search for and remove if found | ||||||||
* @tparam T the contained type of the vector | ||||||||
*/ | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
template <typename T> | ||||||||
bool Exclude(std::vector<T>& vec, const T& elem) { | ||||||||
auto it = Find(vec, elem); | ||||||||
if (vec.end() == it) return false; | ||||||||
Erase(vec, it); | ||||||||
return true; | ||||||||
} | ||||||||
|
||||||||
/** Clear a vector and then force it to reduce its capacity. | ||||||||
* @param vec the vector to be modified | ||||||||
* @tparam T the contained type of the vector | ||||||||
*/ | ||||||||
template <typename T> | ||||||||
void Reset(std::vector<T>& vec) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pretty sure this function isn't necessary - can be replaced with a clear() - the capacity reduction was just due to how the allocations worked There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It may be a good idea to keep the capacity reduction where it is used in case that behavior is desired. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
template <typename T>
void Reset(std::vector<T>& vec) {
std::vector<T>{}.swap(vec);
} |
||||||||
vec.clear(); | ||||||||
vec.shrink_to_fit(); | ||||||||
} | ||||||||
|
||||||||
/** Extend a vector by an amount and return an iterator to the new elements. | ||||||||
* @param vec the vector to be modified | ||||||||
* @param num the number of elements to add | ||||||||
* @tparam T the contained type of the vector | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||||
*/ | ||||||||
template <typename T> | ||||||||
typename std::vector<T>::iterator Extend(std::vector<T>& vec, size_t num) { | ||||||||
vec.resize(vec.size() + num); | ||||||||
return vec.end() - num; | ||||||||
} | ||||||||
|
||||||||
/** | ||||||||
* Simple vector template class. | ||||||||
* | ||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -77,7 +77,7 @@ static inline int32 BigMulS(const int32 a, const int32 b, const uint8 shift) | |||||
return (int32)((int64)a * (int64)b >> shift); | ||||||
} | ||||||
|
||||||
typedef SmallVector<Industry *, 16> SmallIndustryList; | ||||||
using SmallIndustryList = std::vector<Industry *> ; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. extra space before There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
/** | ||||||
* Score info, values used for computing the detailed performance rating. | ||||||
|
@@ -1057,7 +1057,7 @@ static uint DeliverGoodsToIndustry(const Station *st, CargoID cargo_type, uint n | |||||
if (IndustryTemporarilyRefusesCargo(ind, cargo_type)) continue; | ||||||
|
||||||
/* Insert the industry into _cargo_delivery_destinations, if not yet contained */ | ||||||
_cargo_delivery_destinations.Include(ind); | ||||||
Include(_cargo_delivery_destinations, ind); | ||||||
|
||||||
uint amount = min(num_pieces, 0xFFFFU - ind->incoming_cargo_waiting[cargo_index]); | ||||||
ind->incoming_cargo_waiting[cargo_index] += amount; | ||||||
|
@@ -1930,11 +1930,8 @@ void LoadUnloadStation(Station *st) | |||||
} | ||||||
|
||||||
/* Call the production machinery of industries */ | ||||||
const Industry * const *isend = _cargo_delivery_destinations.End(); | ||||||
for (Industry **iid = _cargo_delivery_destinations.Begin(); iid != isend; iid++) { | ||||||
TriggerIndustryProduction(*iid); | ||||||
} | ||||||
_cargo_delivery_destinations.Clear(); | ||||||
for (const auto &iid : _cargo_delivery_destinations) TriggerIndustryProduction(iid); | ||||||
_cargo_delivery_destinations.clear(); | ||||||
} | ||||||
|
||||||
/** | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -397,7 +397,7 @@ static void FiosGetFileList(SaveLoadOperation fop, fios_getlist_callback_proc *c | |||||
{ | ||||||
SortingBits order = _savegame_sort_order; | ||||||
_savegame_sort_order = SORT_BY_NAME | SORT_ASCENDING; | ||||||
QSortT(file_list.files.Begin(), file_list.files.Length(), CompareFiosItems); | ||||||
QSortT(file_list.files.data(), file_list.Length(), CompareFiosItems); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit more work involved, but since you're converting some stuff in this PR other than just converting SmallVector, perhaps converting to use std::sort would be a good idea as well Alternatively, it should be a separate PR (along with the various other changes in this PR) :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that the standard algorithms would be ideal where applicable, a seperate PR would make most sense. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here an untested suggestion: (at least
Suggested change
|
||||||
_savegame_sort_order = order; | ||||||
} | ||||||
|
||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -113,7 +113,7 @@ class FileList { | |||||
*/ | ||||||
inline FiosItem *Append() | ||||||
{ | ||||||
return this->files.Append(); | ||||||
return &*Extend(this->files, 1); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ew. Looks like FileList is itself a pseudo-SmallVector - should be "converted" ? |
||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -122,7 +122,7 @@ class FileList { | |||||
*/ | ||||||
inline uint Length() const | ||||||
{ | ||||||
return this->files.Length(); | ||||||
return this->files.size(); | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -131,7 +131,7 @@ class FileList { | |||||
*/ | ||||||
inline const FiosItem *Begin() const | ||||||
{ | ||||||
return this->files.Begin(); | ||||||
return this->files.data(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure the vector contains files.
Suggested change
|
||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -140,7 +140,7 @@ class FileList { | |||||
*/ | ||||||
inline const FiosItem *End() const | ||||||
{ | ||||||
return this->files.End(); | ||||||
return this->Begin() + this->Length(); | ||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -149,7 +149,7 @@ class FileList { | |||||
*/ | ||||||
inline const FiosItem *Get(uint index) const | ||||||
{ | ||||||
return this->files.Get(index); | ||||||
return this->files.data() + index; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
/** | ||||||
|
@@ -158,7 +158,7 @@ class FileList { | |||||
*/ | ||||||
inline FiosItem *Get(uint index) | ||||||
{ | ||||||
return this->files.Get(index); | ||||||
return this->files.data() + index; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
} | ||||||
|
||||||
inline const FiosItem &operator[](uint index) const | ||||||
|
@@ -178,19 +178,19 @@ class FileList { | |||||
/** Remove all items from the list. */ | ||||||
inline void Clear() | ||||||
{ | ||||||
this->files.Clear(); | ||||||
this->files.clear(); | ||||||
} | ||||||
|
||||||
/** Compact the list down to the smallest block size boundary. */ | ||||||
inline void Compact() | ||||||
{ | ||||||
this->files.Compact(); | ||||||
this->files.shrink_to_fit(); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use the "swap-trick" to force the shrink.
Suggested change
|
||||||
} | ||||||
|
||||||
void BuildFileList(AbstractFileType abstract_filetype, SaveLoadOperation fop); | ||||||
const FiosItem *FindItem(const char *file); | ||||||
|
||||||
SmallVector<FiosItem, 32> files; ///< The list of files. | ||||||
std::vector<FiosItem> files; ///< The list of files. | ||||||
}; | ||||||
|
||||||
enum SortingBits { | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ char *_hotkeys_file; | |
* List of all HotkeyLists. | ||
* This is a pointer to ensure initialisation order with the various static HotkeyList instances. | ||
*/ | ||
static SmallVector<HotkeyList*, 16> *_hotkey_lists = NULL; | ||
static std::vector<HotkeyList*> *_hotkey_lists = NULL; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. pointer to a vector of pointers... lovely. I see from the comment that it was for initialisation order reasons, but is that still necessary? |
||
|
||
/** String representation of a keycode */ | ||
struct KeycodeNames { | ||
|
@@ -202,7 +202,7 @@ const char *SaveKeycodes(const Hotkey *hotkey) | |
{ | ||
static char buf[128]; | ||
buf[0] = '\0'; | ||
for (uint i = 0; i < hotkey->keycodes.Length(); i++) { | ||
for (uint i = 0; i < hotkey->keycodes.size(); i++) { | ||
const char *str = KeycodeToString(hotkey->keycodes[i]); | ||
if (i > 0) strecat(buf, ",", lastof(buf)); | ||
strecat(buf, str, lastof(buf)); | ||
|
@@ -247,19 +247,19 @@ Hotkey::Hotkey(const uint16 *default_keycodes, const char *name, int num) : | |
*/ | ||
void Hotkey::AddKeycode(uint16 keycode) | ||
{ | ||
this->keycodes.Include(keycode); | ||
Include(this->keycodes, keycode); | ||
} | ||
|
||
HotkeyList::HotkeyList(const char *ini_group, Hotkey *items, GlobalHotkeyHandlerFunc global_hotkey_handler) : | ||
global_hotkey_handler(global_hotkey_handler), ini_group(ini_group), items(items) | ||
{ | ||
if (_hotkey_lists == NULL) _hotkey_lists = new SmallVector<HotkeyList*, 16>(); | ||
*_hotkey_lists->Append() = this; | ||
if (_hotkey_lists == NULL) _hotkey_lists = new std::vector<HotkeyList*>(); | ||
_hotkey_lists->push_back(this); | ||
} | ||
|
||
HotkeyList::~HotkeyList() | ||
{ | ||
_hotkey_lists->Erase(_hotkey_lists->Find(this)); | ||
Exclude(*_hotkey_lists, this); | ||
} | ||
|
||
/** | ||
|
@@ -272,7 +272,7 @@ void HotkeyList::Load(IniFile *ini) | |
for (Hotkey *hotkey = this->items; hotkey->name != NULL; ++hotkey) { | ||
IniItem *item = group->GetItem(hotkey->name, false); | ||
if (item != NULL) { | ||
hotkey->keycodes.Clear(); | ||
hotkey->keycodes.clear(); | ||
if (item->value != NULL) ParseHotkeys(hotkey, item->value); | ||
} | ||
} | ||
|
@@ -300,7 +300,7 @@ void HotkeyList::Save(IniFile *ini) const | |
int HotkeyList::CheckMatch(uint16 keycode, bool global_only) const | ||
{ | ||
for (const Hotkey *list = this->items; list->name != NULL; ++list) { | ||
if (list->keycodes.Contains(keycode | WKC_GLOBAL_HOTKEY) || (!global_only && list->keycodes.Contains(keycode))) { | ||
if (Contains(list->keycodes, static_cast<uint16>(keycode | WKC_GLOBAL_HOTKEY)) || (!global_only && Contains(list->keycodes, keycode))) { | ||
return list->num; | ||
} | ||
} | ||
|
@@ -313,11 +313,11 @@ static void SaveLoadHotkeys(bool save) | |
IniFile *ini = new IniFile(); | ||
ini->LoadFromDisk(_hotkeys_file, NO_DIRECTORY); | ||
|
||
for (HotkeyList **list = _hotkey_lists->Begin(); list != _hotkey_lists->End(); ++list) { | ||
for (auto &list : *_hotkey_lists) { | ||
if (save) { | ||
(*list)->Save(ini); | ||
list->Save(ini); | ||
} else { | ||
(*list)->Load(ini); | ||
list->Load(ini); | ||
} | ||
} | ||
|
||
|
@@ -340,11 +340,11 @@ void SaveHotkeysToConfig() | |
|
||
void HandleGlobalHotkeys(WChar key, uint16 keycode) | ||
{ | ||
for (HotkeyList **list = _hotkey_lists->Begin(); list != _hotkey_lists->End(); ++list) { | ||
if ((*list)->global_hotkey_handler == NULL) continue; | ||
for (auto &list : *_hotkey_lists) { | ||
if (list->global_hotkey_handler == NULL) continue; | ||
|
||
int hotkey = (*list)->CheckMatch(keycode, true); | ||
if (hotkey >= 0 && ((*list)->global_hotkey_handler(hotkey) == ES_HANDLED)) return; | ||
int hotkey = list->CheckMatch(keycode, true); | ||
if (hotkey >= 0 && (list->global_hotkey_handler(hotkey) == ES_HANDLED)) return; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text should be on a separate line, to preserve alignment
(applies to lots of functions)