search

Wargus / wargus

Importer and scripts for Warcraft II: Tides of Darkness, the expansion Beyond the Dark Portal, and Aleonas Tales
GNU General Public License v2.0
355 stars 55 forks source link

Consider modernizing the C++ code in wartool.cpp #398

Open ghost opened 2 years ago

ghost commented 2 years ago

Reading through the code in wartool.cpp, I am noticing a lot of raw pointer manipulations, mallocs, and frees. Since the cmake file sets the C++ standard to C++17, it might make sense to modernize some of the code.

As one example, unsigned char* ExtractEntry(unsigned char* cp, size_t* lenp) is used alot throughout the code, and if we were wanting to use something closer to idiomatic C++17 it would probably return a std::vector<unsigned char>. Even if we were wanting to continue using pointers, the result of ExtractEntry seems to be used at most once, so changing it to a std::unique_ptr<unsigned char[]> as the return value would probably make sense.

There are of course other cases, but that function seems like a reasonable starting point since it is used so often. More broadly changing the IO functions from printfs and null terminated strings to std::strings and iostreams would probably make sense as well since there are a largish number of string manipulations I saw.

Let me know if there is any interest in making changes like that, or if anyone has any other concerns or ideas about a change like that.

ipochto commented 2 years ago

Hi. Yep, you are right. There is a lot of legacy code from times when wargus/staratagus was wrote on pure C. But it works mostly. In a situation of lack of resources (mainly time, and developers of course)... imho the polishing of code while there is another issues/improvements which must to be fixed to make stratagus realy playable is not really rational )

For example speaking of std::shared_ptr: There is a mechanism of refs - every unit has ref-counter which counts how many another units refers to this unit. From time to time asserts related to this refs is triggered. There is a need to rewrite this part on shared_ptr to remove this source of bugs. @Andrettin made this changes for wyrmgus (which has a huge common code base with stratagus) but... see my sentence about resources :))

But it just imho. Anyway every improvement will be appreciated.

ipochto commented 2 years ago

By the way, in case if you don't know - there is a discord channel. https://discord.gg/dQGxaw3QfB

ghost commented 2 years ago

Cool, I did not know about the discord channel. Thanks for the response, it wasn't super clear if there was some coding guidelines I missed. I know there are a decent number of people and projects that prefer to write C with classes style C++.