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

Support stl-array #757

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Support stl-array #757

merged 1 commit into from
Jun 7, 2024

Conversation

lethosor
Copy link
Member

Companion to DFHack/dfhack#4591

Not necessary to merge until we need it.

@ab9rf
Copy link
Member

ab9rf commented May 19, 2024

we have been generally been rewriting std::array uses in bay12 headers as <static-array> because the implementation of std::array<T,sz> in both gcc and msvc is binary-equivalent to that of T[sz], so std::array isn't strictly necessary for the purposes of structures, but at the same time it allows for a more transparent respresentation of bay12 source documents. at the same time it's not likely to be harmful and i don't think it adds much to the maintainability burden, and it could be useful if we ever write something to directly parse bay12 header files....

@lethosor
Copy link
Member Author

lethosor commented May 19, 2024

This was mostly intended to test the std::array specialization of generic_container_identity. I'm fine delaying this until a point when we do want std::array support.

One benefit of std::array is that bounds checking is available in C++, with .at(). static-array only provides checking to Lua.

@ab9rf
Copy link
Member

ab9rf commented May 19, 2024

One benefit of std::array is that bounds checking is available in C++, with .at(). static-array only provides checking to Lua.

I'd actually be fine with using std::array for all static arrays in the DF structures, in part because it provides bounds checking.

Copy link
Member

@ab9rf ab9rf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, subject to comments in prior discussion

@lethosor
Copy link
Member Author

To be clear - operator[] does not provide bounds checking, similar to std::vector. Only .at() does, and most of our code doesn't use .at().

@chdoc
Copy link
Member

chdoc commented May 21, 2024

I don't know about MSVC, but for GCC, one can choose to compile operator[] with bounds checking:

Macros

_GLIBCXX_ASSERTIONS
Undefined by default. When defined, enables extra error checking in the form of precondition assertions, such as bounds checking in strings and null pointer checks when dereferencing smart pointers.

(even though it isn't mentioned, this includes array)

@myk002
Copy link
Member

myk002 commented May 28, 2024

This was mostly intended to test the std::array specialization of generic_container_identity. I'm fine delaying this until a point when we do want std::array support.

I'd prefer to merge it if it makes sense. A forever-open PR is just bound to get out of sync with the main branch

@myk002 myk002 merged commit 3e289b9 into DFHack:master Jun 7, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants