MantisBT - Zandronum
View Issue Details
IDProjectCategoryView StatusDate SubmittedLast Update
0001820Zandronum[All Projects] Suggestionpublic2014-06-08 00:142020-05-05 21:45
ReporterEdward-san 
Assigned To 
PrioritynormalSeveritytweakReproducibilityalways
StatusclosedResolutioninvalid 
PlatformOSOS Version
Product Version1.2 
Target VersionFixed in Version 
Summary0001820: [refactoring] Replace std:: classes with internal zdoom classes
DescriptionNothing much to say, except for the fact that with these changes, some overheads from implementation-dependent part of std are gone for good.

I hope this brings a lot of improvements in code size and in compile-time and runtime speed.

I tested some of the changes, like the upnpnat and the outbound traffic measurements and found no problems so far.

Also note that some changes are in win32 folder and are not accessible to me by compiling or anything, so tell me what should be changed.

This patch is for the 'stable' branch, though I know it may not be committed there. In order to apply to 2.0, the 4th hunk of p_mobj.cpp part should be removed, because the changes involved two separated versions of AActor::SetState, which were merged in 2.0.
Steps To Reproduce
Additional InformationEssentially:
std::string is replaced by FString;
all the containers, like lists,vectors and sets, are replaced by TArray;
std::map is replaced by TMap, with the side effect that the ordering of the keys is changed (TMap is a hash map, while std::map is an ordered map), so 'dumptrafficmeasure' ccmd may report the actors list in a different order;
std::pair is only used with two strings, so I replaced it with a simple newly-created StringPair;
removed some standard headers as consequence of these above.
TagsNo tags attached.
Relationships
Attached Filesdiff huge_std_convert.diff (89,405) 2014-06-08 00:14
/tracker/file_download.php?file_id=1219&type=bug
Issue History
Date ModifiedUsernameFieldChange
2014-06-08 00:14Edward-sanNew Issue
2014-06-08 00:14Edward-sanStatusnew => assigned
2014-06-08 00:14Edward-sanAssigned To => Edward-san
2014-06-08 00:14Edward-sanFile Added: huge_std_convert.diff
2014-06-08 00:14Edward-sanStatusassigned => feedback
2014-06-08 00:21DuskStatusfeedback => needs review
2014-06-08 00:47WatermelonNote Added: 0008896
2014-06-08 00:48WatermelonNote Edited: 0008896bug_revision_view_page.php?bugnote_id=8896#r4831
2014-06-08 00:48WatermelonNote Edited: 0008896bug_revision_view_page.php?bugnote_id=8896#r4832
2014-06-08 07:51Torr SamahoNote Added: 0008903
2014-06-08 07:52Torr SamahoStatusneeds review => feedback
2014-06-15 11:10Edward-sanNote Added: 0009369
2014-06-15 11:10Edward-sanStatusfeedback => assigned
2014-06-15 17:43Torr SamahoNote Added: 0009471
2014-06-15 19:16Edward-sanNote Added: 0009478
2014-06-15 19:19Edward-sanNote Edited: 0009478bug_revision_view_page.php?bugnote_id=9478#r4992
2014-06-15 19:24Torr SamahoNote Added: 0009479
2020-05-05 21:45Edward-sanNote Added: 0021311
2020-05-05 21:45Edward-sanStatusassigned => closed
2020-05-05 21:45Edward-sanAssigned ToEdward-san =>
2020-05-05 21:45Edward-sanResolutionopen => invalid

Notes
(0008896)
Watermelon   
2014-06-08 00:47   
(edited on: 2014-06-08 00:48)
As for a big discussion on this, hopefully the following questions can be answered:

1) Is it worth forever locking the std libraries out?
There are a ton of useful features that I've grown accustom to over the years of using the std library. One I really liked that they added was std::regex. If we ever upgrade to C++ 11 compatibility, we'd never get to use this because we have deviated from using any std libraries.

2) The overhead, is it worth it?
Since you did all the changes, can you see how much size gain we get? Is it only 10-20 kb? or is it much more?

3) Someone needs to document the equivalents of what is in ZDoom, such as (some of these we already know, like FString):
String
HashMap
Sets
Queue/Deque
Priority Queue
Stack
Tree
Iterators (for the above)

I'm also worried they won't have some of the useful std lib features. While I can always code them in and add them to ZDoom... I'd have to wait for a backport to get it since it'd be way out of order.


----------------------------------------------------------

I'm not against switching to all ZDoom containers. If they are easy to work with and allow me to do what I normally can do, I personally wouldn't care at all.

It looks like you did a lot of work there, so I don't want to sound like I'm against it. Just playing the Devil's advocate ;)


I would definitely love the documentation (just a short one somewhere) so that I don't have to waste time trying to find out what each one does...etc.
(0008903)
Torr Samaho   
2014-06-08 07:51   
Quote from Edward-san
I hope this brings a lot of improvements in code size and in compile-time and runtime speed.

Do you have any evidence that this is actually the case?

So far I don't see any real benefit in getting rid of the std classes. In some cases I even intentionally did not use the ZDoom classes. For instance, I did not use FString anywhere in networkshared.h and networkshared.cpp so that this code can be used outside of ZDoom code, e.g. in the master server.
(0009369)
Edward-san   
2014-06-15 11:10   
Okay, I reverted the changes on networkshared.*, but I have some concerns about some things:

1) the usage of std::ostream in the function QueryIPQueue::addAddress . Why don't you use "FILE *" and the "fprintf" function? (the string you print has no format, so there's no overhead from runtime checking, to begin with!);

2) std::stringstream inside some functions, when you can use safely just std::string operator+ and operator+= (all the arguments passed to '<<' are compatible with the std::string operators).

3) I can remove some useless includes (considering also point 1 and 2):

//from networkshared.cpp
#include <sstream>
#include <errno.h>
#include <iostream>

//from networkshared.h
#include <string.h>
#include <stdlib.h>
#include <list>
#include <ctype.h>
#include <math.h>


Obviously, 1) implies that masterserver should be changed by using "stderr" instead of "&std::cerr".

I'll make a patch with these changes.
(0009471)
Torr Samaho   
2014-06-15 17:43   
Quote from Edward-san

1) the usage of std::ostream in the function QueryIPQueue::addAddress . Why don't you use "FILE *" and the "fprintf" function? (the string you print has no format, so there's no overhead from runtime checking, to begin with!);
Why should I? We are not restricted to C code, so I can nicely use the STL. As I said above, I don't see any real benefit in getting rid of the STL.
Quote from Edward-san

2) std::stringstream inside some functions, when you can use safely just std::string operator+ and operator+= (all the arguments passed to '<<' are compatible with the std::string operators).

Why? The code is a perfectly valid use of std::stringstream. I would even dare to say that it's more efficient than using the std::string operator+ since each call of the latter creates an extra string copy.
(0009478)
Edward-san   
2014-06-15 19:16   
(edited on: 2014-06-15 19:19)
Quote from Torr_Samaho

I would even dare to say that it's more efficient than using the std::string operator+ since each call of the latter creates an extra string copy.


If there's enough reserved memory for all the strings to append to the std::string object, why should it?
(0009479)
Torr Samaho   
2014-06-15 19:24   
By design operator+ has to return a temporary copy of the combined string. That copy then is later stored into whatever you use operator= on. You don't have the problem with operator+=, but there you need multiple lines to add multiple strings.

Anyway, even if using a string is as good as a stringstream here (the compiler optimizations can possibly eliminate the temporary copy), I still see absolutely no reason not to use the stringstream.
(0021311)
Edward-san   
2020-05-05 21:45   
Actually, after a long time I realized that this idea is not good. If the internal code must be separated from the game code, this would hinder the process. Closing this.