llde / xOBSE

Oblivion Script extender source
243 stars 35 forks source link

Question: language standard, pull requests. #213

Open calebtt opened 11 months ago

calebtt commented 11 months ago

I would like to contribute some code to this project, but I have a couple of questions:

**1. What C++ language standard are you targeting?

  1. Any advice on pull requests?
  2. Coding standards? C++ core guidelines would be great to follow if you don't have your own.**

I don't intend to make major architectural modifications anytime soon, just some code quality and modernization changes, if the language standard will support that.

calebtt commented 11 months ago

I would point out a possible bug: SetDescriptionTextForForm(...) in Hooks_Gameplay.cpp

The declaration in the header assigned -1 to the unsigned char, which should underflow and give the max value afaik. But the implementation compares the unsigned char to "-1" as an integer, not casted to unsigned char and underflowed to max. My IDE tells me the condition is always false.

I made some changes in a fork of the repo that address that by adding the NOMINMAX define before including Windows.h, and then replacing the one single instance of the macros' usage with std::min and std::max.

https://github.com/calebtt/xOBSE/tree/modernized

There are other changes included in the commit, such as changing static const integral variables to static constexpr, a few instances of changing std::container_type::iterator to "auto", and other things of that nature--replaced NULL with nullptr. Changed a call to .size() to determine if empty to !.empty(), etc.

llde commented 11 months ago

Hi @calebtt the current standard is iirc setted as c++latest that should contains c++17 standard with additions from c++20 standard. I don't care what standard is used, until is supported by vs2019 toolchains, and I ask to not bumb to vs2022 as the ide and its tolchains aren't usable under wine.

For conding standard I'm not sure, I'd like to be consistent. I will provide direct feedback in the pull request analysing cade by case. I will review you changes

llde commented 10 months ago

Hi @calebtt Regarding the issue I commented on #220 It seems related on the std::string creation from std::array While in most place the string is created from the data pointer, in the kDataType_String case it's created from array iterator cbegin cend. This cause the resulting std::string to overallocate (the size is the size of the entire array) and apparently to ignore any Null terminator. This caused the cosave to grow too large and array definitions to be out of bounds. The solution in this case is to properly create the string usind the internal buffer of the std::Array or using as iterator pair (array.begin(), array.begin() + NullTerminatorIndex). This is important in cases you can't use the char array directly (for example creating wstrings, that I think may be affected by the same issue)

calebtt commented 10 months ago

Yes that looks correct, I'm sorry I didn't look into it earlier. Lots of work work.