Modernize: Move some arrays to std::array

GravisZro commented 1 month ago

Pull Request Type

[x] Runtime changes
- [x] 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

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

winterheart commented 1 month ago

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

GravisZro commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

tophyr commented 1 month ago

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 commented 4 weeks ago

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.](https://godbolt.org/#g:!((g:!((g:!((h:codeEditor,i:(filename:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,selection:(endColumn:25,endLineNumber:6,positionColumn:25,positionLineNumber:6,selectionStartColumn:25,selectionStartLineNumber:6,startColumn:25,startLineNumber:6),source:'%23include+%3Carray%3E%0A%0Aint+main()%0A%7B%0A++++std::array%3Cint,+3%3E+arr%3B%0A++++for(int+i+%3D+0%3B+i+%3C+5%3B+i%2B%2B)%0A++++%7B%0A++++++arr%5Bi%5D+%3D+0%3B%0A++++%7D%0A++++return+0%3B%0A%7D'),l:'5',n:'1',o:'C%2B%2B+source+%231',t:'0')),k:50.42378344047672,l:'4',n:'0',o:'',s:0,t:'0'),(g:!((g:!((h:compiler,i:(compiler:gsnapshot,filters:(b:'0',binary:'0',binaryObject:'1',commentOnly:'0',debugCalls:'1',demangle:'0',directives:'1',execute:'0',intel:'1',libraryCode:'0',trim:'1',verboseDemangling:'0'),flagsViewOpen:'1',fontScale:14,fontUsePx:'0',j:1,lang:c%2B%2B,libs:!(),options:'',overrides:!(),selection:(endColumn:1,endLineNumber:1,positionColumn:1,positionLineNumber:1,selectionStartColumn:1,selectionStartLineNumber:1,startColumn:1,startLineNumber:1),source:1),l:'5',n:'0',o:'+x86-64+gcc+(trunk)+(Editor+%231)',t:'0')),k:49.57621655952329,l:'4',m:62.73339179622393,n:'0',o:'',s:0,t:'0'),(g:!((h:output,i:(compilerName:'x86-64+gcc+(trunk)',editorid:1,fontScale:14,fontUsePx:'0',j:1,wrap:'1'),l:'5',n:'0',o:'Output+of+x86-64+gcc+(trunk)+(Compiler+%231)',t:'0')),header:(),l:'4',m:37.26660820377608,n:'0',o:'',s:0,t:'0')),k:49.57621655952329,l:'3',n:'0',o:'',t:'0')),l:'2',n:'0',o:'',t:'0')),version:4)

DescentDevelopers / Descent3