Modernize: Move some arrays to std::array #412

GravisZro · 2024-06-01T17:53:49Z

Pull Request Type

Runtime changes
- Other changes

Description

The ultimate goal is to switch several variables to vectors in order to expand game limitations but the first step is to switch C arrays to std::array so that we ensure that memory bounds are respected. There is more in-depth work to adapting how the arrays are used but I didn't want to make the PR too large.

Other stuff

The SAVE_DATA_TABLE was replaced with a template function version
The OBJNUM macro was used in place of obj - Objects
Three bugs found and fixed.
- In ObjInitGeneric in for loop where obj_info wasn't being used.
- Twice in WeaponFire.cpp where fq.thisobjnum was being assigned Objects - obj instead of obj - Objects (replaced with OBJNUM macro)

Checklist

I have tested my changes locally and verified that they work as intended.
I have documented any new or modified functionality.
I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Also replaces a macro with a template function

winterheart · 2024-06-02T00:33:24Z

I don't see value of these changes. If you trying to solve particular issue, focus on particular part of code. General rule of thumb: "Don't fix that aren't broken".

Bugfixes - In `ObjInitGeneric` in for loop where `obj_info` wasn't being used. - Twice in WeaponFire.cpp where `fq.thisobjnum` was being assigned `Objects - obj` instead of `obj - Objects`

GravisZro · 2024-06-02T01:06:10Z

I don't see value of these changes.

That's your mistake because there are currently significant issues open that are likely due to memory access issues.

winterheart · 2024-06-02T01:09:58Z

I don't see value of these changes.

That's your mistake because there are currently significant issues open that are likely due to memory access issues.

Prove it, please. Do you have checked code with Valgrind, is there memory issues on changed code? Do all changed in this PR code suffer these issues?

GravisZro · 2024-06-02T01:25:03Z

The purpose of the changes are to ensure that if a memory violation occurs that it will immediately be caught rather than continue, possibly crashing at a later point. I don't understand why you are being so oppositional to improving the code's reliability. Also, I did point out that I found and fixed some bugs while doing this.

GravisZro · 2024-06-06T15:57:44Z

@winterheart
Do you not want bugfixes? This contains bugfixes.

tophyr · 2024-06-07T21:59:30Z

Descent3/AImain.cpp

@@ -1625,7 +1625,7 @@ const char *const Ai_movement_subtype_walking_strings[MAX_AI_INIT_MOVEMENT_SUBTY
 #define AI_MAX_MELEE_RANGE 5.0f

 int AI_NumRendered;
-int AI_RenderedList[MAX_OBJECTS];
+std::array<int, MAX_OBJECTS> AI_RenderedList;


As long as we're doing these conversions, let's default-initialize the elements in the arrays:

Suggested change

std::array<int, MAX_OBJECTS> AI_RenderedList;

std::array<int, MAX_OBJECTS> AI_RenderedList{};

Both T[N] and std::array<T, N> default-initialize only the array object, not the array elements. IE, this gives an indeterminate value:

std::array<int, 4> arr; int val = arr[0]; // val is indeterminate

I mean... OK but I was expecting most of these to eventually be changed to std::vector.

Vectors would be preferable, definitely. I'm fine with not adding the explicit initialization in this changeset.

Descent3/Game2DLL.cpp

Descent3/ObjInit.cpp

tophyr

noticed what i think is a problem in scripts/LEVEL15.cpp - added array initialization objects. maybe some find-replace artifacts?

don't love the added [[gnu::packed]] attribute in Descent3/aistruct.h either.

beyond the above, added some suggestions but this looks largely fine to me.

Descent3/WeaponFire.cpp

tophyr · 2024-06-07T22:08:14Z

Descent3/aistruct.h

@@ -713,17 +715,17 @@ struct ain_weapon_hit_info {

 #define AI_MEM_DEPTH 5

-struct ai_mem {
+struct [[gnu::packed]] ai_mem {


why is struct packing needed? without context, this seems smelly

Also [[gnu::packed]] isn't portable, MSVC will just ignore the unknown attribute with warning C5030: attribute [[gnu::packed]] is not recognized.
I believe #pragma pack is supported by all recent compiler versions nowadays.

@tophyr this was part of something else I was working on.
@pzychotic MSVC is perpetually behind both GCC and clang. You would be better off using clang/llvm on windows.

It isn't about what I use, it's about what this project does. I linked to the current CI build log, because we do use MSVC here and that attribute is being ignored in our windows builds. Unless this projects decides to be GCC/Clang only, this issue remains.
Also don't try to tell me which build environment I have to use, just because you don't like it.

not really a problem because it's being removed.

Descent3/aistruct.h

Descent3/fireball.cpp

Descent3/gamesave.cpp

Descent3/multi.h

scripts/LEVEL15.cpp

tophyr · 2024-06-07T22:28:39Z

The purpose of the changes are to ensure that if a memory violation occurs that it will immediately be caught rather than continue

@GravisZro for what it's worth, I didn't notice any changes here (other than the use of std::array::fill() over memset()) that actually add bounds-checking. std::array doesn't perform bounds checking via its iterators or operator[]. I do think that we should move to std::array; I just want to call out that the type itself adds no inherent safety. The additional safety will come when we transition to iterator for loops (for (auto const& foo : foos) etc) and stdlib algorithms.

GravisZro · 2024-06-09T01:42:39Z

std::array doesn't perform bounds checking via its iterators or operator[]

The C++ standard doesn't not specify the behavior of accessing nonexistent elements using operator[]. However, common implementations like GCC's libstdc++ will do bounds checking.

tophyr

This is all great, modulo the [[gnu::packed]] annotation.

scripts/LEVEL15.cpp

jengelh · 2024-08-30T00:33:51Z

std::array doesn't perform bounds checking via its iterators or operator[]

The C++ standard doesn't not specify the behavior of accessing nonexistent elements using operator[]. However, common implementations like GCC's libstdc++ will do bounds checking.

But also only in specific debug builds (-D_GLIBCXX_DEBUG=1). For guaranteed bounds checking, there has always been at().

GravisZro force-pushed the modernize/array branch from 77ffd76 to 8f911c9 Compare June 1, 2024 22:10

Modernize: Move some arrays to std::array

910710e

Also replaces a macro with a template function

GravisZro force-pushed the modernize/array branch 7 times, most recently from cd12329 to 86ec6c3 Compare June 1, 2024 23:06

More of the same

a46f0f0

GravisZro force-pushed the modernize/array branch from 86ec6c3 to a46f0f0 Compare June 1, 2024 23:15

Event more... with bugfixes!

e02469e

Bugfixes - In `ObjInitGeneric` in for loop where `obj_info` wasn't being used. - Twice in WeaponFire.cpp where `fq.thisobjnum` was being assigned `Objects - obj` instead of `obj - Objects`

GravisZro marked this pull request as ready for review June 2, 2024 01:06

tophyr reviewed Jun 7, 2024

View reviewed changes

Descent3/Game2DLL.cpp Show resolved Hide resolved

tophyr reviewed Jun 7, 2024

View reviewed changes

Descent3/Game2DLL.cpp Show resolved Hide resolved

tophyr reviewed Jun 7, 2024

View reviewed changes

Descent3/ObjInit.cpp Show resolved Hide resolved

tophyr suggested changes Jun 7, 2024

View reviewed changes

tophyr approved these changes Jun 9, 2024

View reviewed changes

scripts/LEVEL15.cpp Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modernize: Move some arrays to std::array #412

Modernize: Move some arrays to std::array #412

GravisZro commented Jun 1, 2024 •

edited

Loading

winterheart commented Jun 2, 2024

GravisZro commented Jun 2, 2024

winterheart commented Jun 2, 2024

GravisZro commented Jun 2, 2024

GravisZro commented Jun 6, 2024

tophyr Jun 7, 2024

GravisZro Jun 9, 2024

tophyr Jun 9, 2024 •

edited

Loading

tophyr left a comment

tophyr Jun 7, 2024

pzychotic Jun 8, 2024

GravisZro Jun 9, 2024

pzychotic Jun 9, 2024

GravisZro Jun 9, 2024

tophyr commented Jun 7, 2024

GravisZro commented Jun 9, 2024 •

edited

Loading

tophyr left a comment

jengelh commented Aug 30, 2024

	std::array<int, MAX_OBJECTS> AI_RenderedList;
	std::array<int, MAX_OBJECTS> AI_RenderedList{};

Modernize: Move some arrays to std::array #412

Are you sure you want to change the base?

Modernize: Move some arrays to std::array #412

Conversation

GravisZro commented Jun 1, 2024 • edited Loading

Pull Request Type

Description

Checklist

winterheart commented Jun 2, 2024

GravisZro commented Jun 2, 2024

winterheart commented Jun 2, 2024

GravisZro commented Jun 2, 2024

GravisZro commented Jun 6, 2024

tophyr Jun 7, 2024

Choose a reason for hiding this comment

GravisZro Jun 9, 2024

Choose a reason for hiding this comment

tophyr Jun 9, 2024 • edited Loading

Choose a reason for hiding this comment

tophyr left a comment

Choose a reason for hiding this comment

tophyr Jun 7, 2024

Choose a reason for hiding this comment

pzychotic Jun 8, 2024

Choose a reason for hiding this comment

GravisZro Jun 9, 2024

Choose a reason for hiding this comment

pzychotic Jun 9, 2024

Choose a reason for hiding this comment

GravisZro Jun 9, 2024

Choose a reason for hiding this comment

tophyr commented Jun 7, 2024

GravisZro commented Jun 9, 2024 • edited Loading

tophyr left a comment

Choose a reason for hiding this comment

jengelh commented Aug 30, 2024

GravisZro commented Jun 1, 2024 •

edited

Loading

tophyr Jun 9, 2024 •

edited

Loading

GravisZro commented Jun 9, 2024 •

edited

Loading