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

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

Open
ghost opened this issue Nov 20, 2021 · 3 comments
Open

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

ghost opened this issue Nov 20, 2021 · 3 comments

Comments

@ghost
Copy link

ghost commented Nov 20, 2021

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
Copy link
Member

ipochto commented Nov 20, 2021

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
Copy link
Member

ipochto commented Nov 20, 2021

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

@ghost
Copy link
Author

ghost commented Nov 20, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant