PR for Modernizing for C++11: replacing instances of SmallVector #6817
Conversation
::utf16_str ::utf16_to_utf8
Some build system changes will be needed to enforce c++11/14 |
LinkGraphJob::NodeAnnotationVector and GUIBridgeList (in bridge_gui.cpp) are both vectors of non-POD data types so those two are the real important ones to fix. The former does get fixed by these changes, but GUIBridgeList does not, and from my cursory check it might be more complex to fix that one. The reason the contained data is non-POD is because of the inclusion of |
Sorry for the slow responses; we have not forgotten about this PR. I did a fast pass on the code, and "it looks fine". The main issue with patches like this is of course that reviewing takes a bit of time ;) As mentioned earlier, it would also need some changes to the So please bare with us while we prepare for modern standards ;) |
@@ -102,7 +102,18 @@ static void NetworkFindBroadcastIPsInternal(NetworkAddressList *broadcast) // GE | |||
if (ifa->ifa_broadaddr->sa_family != AF_INET) continue; | |||
|
|||
NetworkAddress addr(ifa->ifa_broadaddr, sizeof(sockaddr)); | |||
if (!broadcast->Contains(addr)) *broadcast->Append() = addr; | |||
//if (!broadcast->Contains(addr)) *broadcast->Append() = addr; |
TrueBrain
Jul 22, 2018
Member
I feel that this was by accident?
I feel that this was by accident?
M3Henry
Jul 27, 2018
Author
Contributor
The removal was intentional, lines 108-116 replace it, but I shouldn't have left it commented out.
TBH the following would be much better than a hand rolled loop.
if (broadcast->end() == std::find(broadcast->begin(), broadcast->end(), addr)
broadcast->push_back(addr);
The removal was intentional, lines 108-116 replace it, but I shouldn't have left it commented out.
TBH the following would be much better than a hand rolled loop.
if (broadcast->end() == std::find(broadcast->begin(), broadcast->end(), addr)
broadcast->push_back(addr);
@TrueBrain If the intention is to keep modernizing the code, then I can go over this patch series and do it properly. Looking back on this, it doesn't seem I put forward my best. |
First pass at an actual review, mostly codestyle related. Firstly, great job on actually getting this done and working - it's a huge amount of effort (I tried SmallMap once, I gave up pretty quickly) However, if I'm honest, I'm not sure this will ever get merged in its current form. There are just too many separate changes all bundled together. It would be better if it were a separate commit per change, rather than per file/type. Something like a commit to add new adapter functions, then one that actually converts types & functions (huge commit, but fundamentally quite simple to read through), then followed by a few commits that "neaten" usage (for loops, auto, etc) Couple general comments:
|
@@ -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. |
LordAro
Sep 19, 2018
Member
text should be on a separate line, to preserve alignment
(applies to lots of functions)
text should be on a separate line, to preserve alignment
(applies to lots of functions)
* @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) { |
LordAro
Sep 19, 2018
Member
opening brace of a function should be on a newline
(applies to various functions)
opening brace of a function should be on a newline
(applies to various functions)
fsimonis
Jan 23, 2019
Contributor
Suggested change
typename std::vector<T>::const_iterator Find(const std::vector<T>& vec, const T& elem) {
typename std::vector<T>::const_iterator Find(const std::vector<T>& vec, const T& elem)
{
typename std::vector<T>::const_iterator Find(const std::vector<T>& vec, const T& elem) { | |
typename std::vector<T>::const_iterator Find(const std::vector<T>& vec, const T& elem) | |
{ |
* @param vec the vector to be searched | ||
* @param elem the element to search for | ||
* @tparam T the contained type of the vector | ||
*/ |
LordAro
Sep 19, 2018
Member
missing @return
doxygen command
(applies to various functions)
missing @return
doxygen command
(applies to various functions)
fsimonis
Jan 23, 2019
Contributor
Suggested change
*/
* @return an iterator to the position of the element if it was found. vec.end() otherwise.
*/
*/ | |
* @return an iterator to the position of the element if it was found. vec.end() otherwise. | |
*/ |
@@ -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 *> ; |
LordAro
Sep 19, 2018
Member
extra space before ;
extra space before ;
fsimonis
Jan 23, 2019
Contributor
Suggested change
using SmallIndustryList = std::vector<Industry *> ;
using SmallIndustryList = std::vector<Industry *>;
using SmallIndustryList = std::vector<Industry *> ; | |
using SmallIndustryList = std::vector<Industry *>; |
@@ -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); |
LordAro
Sep 19, 2018
Member
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) :)
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) :)
M3Henry
Sep 19, 2018
•
Author
Contributor
I agree that the standard algorithms would be ideal where applicable, a seperate PR would make most sense.
I agree that the standard algorithms would be ideal where applicable, a seperate PR would make most sense.
fsimonis
Jan 23, 2019
•
Contributor
Here an untested suggestion: (at least #include<algorithm>
is missing)
Suggested change
QSortT(file_list.files.data(), file_list.Length(), CompareFiosItems);
std::sort(file_list.files.data(), file_list.files.data() + file_list.Length(), CompareFiosItems);
Here an untested suggestion: (at least #include<algorithm>
is missing)
QSortT(file_list.files.data(), file_list.Length(), CompareFiosItems); | |
std::sort(file_list.files.data(), file_list.files.data() + file_list.Length(), CompareFiosItems); |
/** Get the end of the content inf iterator. */ | ||
ConstContentIterator End() const { return this->infos.End(); } | ||
ConstContentIterator End() const { return this->infos.end(); } |
LordAro
Sep 19, 2018
Member
This looks like another class that could be refactored to be "better", instead of reimplementing a load of std::vector functions
This looks like another class that could be refactored to be "better", instead of reimplementing a load of std::vector functions
} | ||
else ++ts; |
LordAro
Sep 19, 2018
Member
braces required here
braces required here
w->lateness_counter = 0; | ||
ClrBit(w->vehicle_flags, VF_TIMETABLE_STARTED); | ||
/* Do multiplication, then division to reduce rounding errors. */ | ||
w->timetable_start = start_date + idx * total_duration / num_vehs / DAY_TICKS; | ||
w->timetable_start = start_date + idx++ * total_duration / num_vehs / DAY_TICKS; |
LordAro
Sep 19, 2018
Member
idx++
in the middle is a bit scary, should probably be in a separate statement
idx++
in the middle is a bit scary, should probably be in a separate statement
* @tparam T the contained type of the vector | ||
*/ | ||
template <typename T> | ||
void Reset(std::vector<T>& vec) { |
LordAro
Sep 19, 2018
Member
pretty sure this function isn't necessary - can be replaced with a clear() - the capacity reduction was just due to how the allocations worked
pretty sure this function isn't necessary - can be replaced with a clear() - the capacity reduction was just due to how the allocations worked
M3Henry
Sep 20, 2018
Author
Contributor
It may be a good idea to keep the capacity reduction where it is used in case that behavior is desired.
According to cppreference: "Leaves the capacity() of the vector unchanged"
It may be a good idea to keep the capacity reduction where it is used in case that behavior is desired.
According to cppreference: "Leaves the capacity() of the vector unchanged"
fsimonis
Jan 23, 2019
Contributor
shrink_to_fit
is only a request which may be ignored.
The guaranteed implementation looks like this:
template <typename T>
void Reset(std::vector<T>& vec) {
std::vector<T>{}.swap(vec);
}
shrink_to_fit
is only a request which may be ignored.
The guaranteed implementation looks like this:
template <typename T>
void Reset(std::vector<T>& vec) {
std::vector<T>{}.swap(vec);
}
@LordAro |
We recently switched from Jenkins as CI to Azure Pipelines as CI. This means you need to rebase before this Pull Request will pass its checks. Sorry for the troubles! (might not be applicable to this PR) |
@M3Henry Yes I think several smaller PRs would be easier to integrate. With the new build infrastructure I think usage of C++11 and possibly C++14 is good to go too. |
That's good to hear,
I've been putting off finishing this because such a large PR needs some
contiguous time to go over.
I may be able to put up a first batch this weekend if the small PR
direction is desirable.
…On Mon, 21 Jan 2019 at 15:26, Niels Martin Hansen ***@***.***> wrote:
@M3Henry <https://github.com/M3Henry> Yes I think several smaller PRs
would be easier to integrate.
With the new build infrastructure I think usage of C++11 and possibly
C++14 is good to go too.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6817 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGyG_kfpt1Jxlr-BDYhq4rx4z1fbbAQzks5vFdwcgaJpZM4Uc1hT>
.
--
Regards,
Henry.
|
* @param vec the vector to be searched | ||
* @param elem the element to search for | ||
* @tparam T the contained type of the vector | ||
*/ |
fsimonis
Jan 23, 2019
Contributor
Suggested change
*/
* @return true if vec contains elem. false otherwise.
*/
*/ | |
* @return true if vec contains elem. false otherwise. | |
*/ |
* @param vec the vector to be searched | ||
* @param elem the element to count instances of | ||
* @tparam T the contained type of the vector | ||
*/ |
fsimonis
Jan 23, 2019
Contributor
Suggested change
*/
* @return the number of occurrences.
*/
*/ | |
* @return the number of occurrences. | |
*/ |
* @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 | ||
*/ |
fsimonis
Jan 23, 2019
Contributor
Suggested change
*/
* @return true if the element had to be added.
*/
*/ | |
* @return true if the element had to be added. | |
*/ |
/** 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 |
fsimonis
Jan 23, 2019
Contributor
Suggested change
* @tparam T the contained type of the vector
* @tparam T the contained type of the vector
* @return the iterator it
* @tparam T the contained type of the vector | |
* @tparam T the contained type of the vector | |
* @return the iterator it |
* @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 | ||
*/ |
fsimonis
Jan 23, 2019
Contributor
Suggested change
*/
* @return true if the element was removed, false if it was not found.
*/
*/ | |
* @return true if the element was removed, false if it was not found. | |
*/ |
/** 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 |
fsimonis
Jan 23, 2019
Contributor
Suggested change
* @tparam T the contained type of the vector
* @tparam T the contained type of the vector
* @return an iterator to the first of the added elements.
* @tparam T the contained type of the vector | |
* @tparam T the contained type of the vector | |
* @return an iterator to the first of the added elements. |
@@ -131,7 +131,7 @@ class FileList { | |||
*/ | |||
inline const FiosItem *Begin() const | |||
{ | |||
return this->files.Begin(); | |||
return this->files.data(); |
fsimonis
Jan 23, 2019
•
Contributor
Make sure the vector contains files.
Suggested change
return this->files.data();
return &(this->files->at(0));
Make sure the vector contains files.
return this->files.data(); | |
return &(this->files->at(0)); |
@@ -149,7 +149,7 @@ class FileList { | |||
*/ | |||
inline const FiosItem *Get(uint index) const | |||
{ | |||
return this->files.Get(index); | |||
return this->files.data() + index; |
fsimonis
Jan 23, 2019
Contributor
Suggested change
return this->files.data() + index;
return &(this->files.at(index));
return this->files.data() + index; | |
return &(this->files.at(index)); |
@@ -158,7 +158,7 @@ class FileList { | |||
*/ | |||
inline FiosItem *Get(uint index) | |||
{ | |||
return this->files.Get(index); | |||
return this->files.data() + index; |
fsimonis
Jan 23, 2019
Contributor
Suggested change
return this->files.data() + index;
return &(this->files.at(index));
return this->files.data() + index; | |
return &(this->files.at(index)); |
} | ||
|
||
/** Compact the list down to the smallest block size boundary. */ | ||
inline void Compact() | ||
{ | ||
this->files.Compact(); | ||
this->files.shrink_to_fit(); |
fsimonis
Jan 23, 2019
Contributor
Use the "swap-trick" to force the shrink.
Suggested change
this->files.shrink_to_fit();
decltype(files)(this->files).swap(this->files);
Use the "swap-trick" to force the shrink.
this->files.shrink_to_fit(); | |
decltype(files)(this->files).swap(this->files); |
After discussions on IRC about modernising the code to make it easier to work on, replacing non time-critical instances of SmallVector<T,S> with std::vector was deemed more practical than making SmallVector STL-like.
This allows use of range-based for loops and STL algorithms to improve expressiveness in the code base.
No changes to functionality have been made.
#6649