Modernize almost all C-style arrays to std::array by xu-shawn · Pull Request #6398 · official-stockfish/Stockfish

xu-shawn · 2025-11-06T16:28:40Z

Passed Non-regression STC
LLR: 2.95 (-2.94,2.94) <-1.75,0.25>
Total: 109280 W: 28619 L: 28482 D: 52179
Ptnml(0-2): 309, 12109, 29719, 12142, 361
https://tests.stockfishchess.org/tests/view/69097b22ea4b268f1fac2a45

No functional change

github-actions · 2025-11-06T16:29:08Z

clang-format 20 needs to be run on this PR.
If you do not have clang-format installed, the maintainer will run it when merging.
For the exact version please see https://packages.ubuntu.com/plucky/clang-format-20.

(execution 19373495928 / attempt 1)

mstembera · 2025-11-06T20:12:52Z

Given that std::array predates Stockfish the original authors and maintainers of the past had ample opportunity to use std:array instead of C arrays if they so chose. However, they haven't and I agree with their decision. C arrays are minimalistic, cleaner, and simpler. This just adds extra lines and unnecessary syntax. I'm very much opposed and consider this an anti simplification.
@xu-shawn Please don't take this personally I think you are a great SF contributor!

anematode · 2025-11-06T20:21:32Z

Agreed with mstembera, while I'm not surprised there's no regression, it just feels messier (especially for multidimensional arrays). And once we take a pointer to it with .data(), there's not really any safety benefit either....

Sopel97 · 2025-11-06T20:34:43Z

I'd suggest a typedef for >1d arrays

I think this is a good change

UniQP · 2025-11-06T19:59:42Z

-    std::uint16_t indices[4];
-    std::memcpy(indices, &b, sizeof(b));
+    std::array<std::uint16_t, 4> indices;
+    std::memcpy(&indices[0], &b, sizeof(b));


Suggested change

std::memcpy(&indices[0], &b, sizeof(b));

std::memcpy(indices.data(), &b, sizeof(b));

UniQP · 2025-11-06T20:00:14Z

        MoveList<CAPTURES> ml(pos);

-        cur = endBadCaptures = moves;
+        cur = endBadCaptures = &moves[0];


Suggested change

cur = endBadCaptures = &moves[0];

cur = endBadCaptures = moves.data();

hmm does this make any difference though? I chose this form because it felt clearer to me

Both versions work but we should keep it consistent and use data() whenever possible (this pull request adds a bunch of data() calls as well).

Some advantages of data():

It makes it more explicit that we are accessing the raw data.

It supports some edge cases where &foo[0] does not work (e.g., https://godbolt.org/z/j7xcz6qer).

It's nicer to read if we have a pointer to a container (e.g., my additional suggestion in src/nnue/network.cpp).

It prevents us from superfluously calling &foo[0] on a raw data pointer (i.e., type* foo) because foo.data() won't compile in this case. This happens a few times in this pull request.

UniQP · 2025-11-06T20:00:38Z


        // Prepare the pointers to loop over the bad captures
-        cur    = moves;
+        cur    = &moves[0];


Suggested change

cur = &moves[0];

cur = moves.data();

UniQP · 2025-11-06T20:00:53Z

        MoveList<EVASIONS> ml(pos);

-        cur    = moves;
+        cur    = &moves[0];


Suggested change

cur = &moves[0];

cur = moves.data();

UniQP · 2025-11-06T20:07:20Z

Could also be improved, while you're at it:

- stream.read(&(*desc)[0], size); + stream.read(desc->data(), size);

- stream.write(&desc[0], desc.size()); + stream.write(desc.data(), desc.size());

UniQP · 2025-11-06T20:58:30Z

    }
    #else
-    std::memcpy(output, biases, sizeof(std::int32_t) * OutputDimensions);
+    std::memcpy(&output[0], &biases[0], sizeof(std::int32_t) * OutputDimensions);


Suggested change

std::memcpy(&output[0], &biases[0], sizeof(std::int32_t) * OutputDimensions);

std::memcpy(output, biases.data(), sizeof(std::int32_t) * OutputDimensions);

UniQP · 2025-11-06T21:03:19Z

+            const auto   input32 = reinterpret_cast<const std::int32_t*>(&input[0]);
+            const vec_t* biasvec = reinterpret_cast<const vec_t*>(&biases[0]);


Suggested change

const auto input32 = reinterpret_cast<const std::int32_t*>(&input[0]);

const vec_t* biasvec = reinterpret_cast<const vec_t*>(&biases[0]);

const auto input32 = reinterpret_cast<const std::int32_t*>(input);

const vec_t* biasvec = reinterpret_cast<const vec_t*>(biases.data());

UniQP · 2025-11-06T21:03:37Z

            }

-            vec_t* outptr = reinterpret_cast<vec_t*>(output);
+            vec_t* outptr = reinterpret_cast<vec_t*>(&output[0]);


Suggested change

vec_t* outptr = reinterpret_cast<vec_t*>(&output[0]);

vec_t* outptr = reinterpret_cast<vec_t*>(output);

UniQP · 2025-11-06T21:11:09Z

        IndexType     count;

-        const auto input32 = reinterpret_cast<const std::int32_t*>(input);
+        const auto input32 = reinterpret_cast<const std::int32_t*>(&input[0]);


Suggested change

const auto input32 = reinterpret_cast<const std::int32_t*>(&input[0]);

const auto input32 = reinterpret_cast<const std::int32_t*>(input);

UniQP · 2025-11-06T21:14:00Z

+            const auto          in        = reinterpret_cast<const __m256i*>(&input[0]);
+            const auto          out       = reinterpret_cast<__m256i*>(&output[0]);


Suggested change

const auto in = reinterpret_cast<const __m256i*>(&input[0]);

const auto out = reinterpret_cast<__m256i*>(&output[0]);

const auto in = reinterpret_cast<const __m256i*>(input);

const auto out = reinterpret_cast<__m256i*>(output);

xu-shawn · 2025-11-06T23:34:02Z

@mstembera the main reason I made this PR is because C-style arrays have becoming a limiting factor when refactoring threat inputs. While it is true that std::array is available for Glaurung authors, modern C++ has come a long way, and is generally regarded now as a superior alternative.

xu-shawn · 2025-11-06T23:41:28Z

@anematode There is actually quite a bit of simplifications std::array makes possible. For example, read_little_endian and similar functions no longer need a size parameter, and memcpy between arrays can be simplified to assignments. I get that multidimensional arrays are syntactically awkward, but we do have a MultiArray class in misc.h, which I can look into later to replace multidimensional arrays.

mstembera · 2025-11-08T00:37:23Z

@mstembera the main reason I made this PR is because C-style arrays have becoming a limiting factor when refactoring threat inputs. While it is true that std::array is available for Glaurung authors, modern C++ has come a long way, and is generally regarded now as a superior alternative.

AFIK it's far from a done deal that threat inputs will make it into master (although i am rooting for it). If it makes it we can include whatever arrays actually need the change then. I disagree that std:array is a superior alternative for OUR use case. If it was the change would have happened long ago.

xu-shawn · 2025-11-08T00:57:42Z

@mstembera You are right that threat inputs is still WIP. My point is that working on threat inputs has revealed, quite clearly, that the current use of C-style arrays are creating unnecessary barriers for extending the code. Even with extensibility aside, std::array is also a great alternative that allows for easier logic (see my reply to anematode), and will only allow for even better safety as we upgrade to C++20 (std::span especially) and beyond.

ddobbelaere · 2025-11-08T13:17:43Z

I like this change, as it makes the codebase more consistent (in using modern C++) and safer, without any runtime effect (same assembly). Arguments of minimalism and simplicity of plain C arrays feel subjective and don't outweigh the benefits.

As an example, consider the ease at which arrays can be memset to zero using .size(), just to name something stupid (it's not stupid!). Previously, there was the risk of providing the wrong size, e.g. after refactoring or patches that change the size.

mstembera · 2025-11-08T19:22:47Z

I like this change, as it makes the codebase more consistent (in using modern C++) and safer, without any runtime effect (same assembly). Arguments of minimalism and simplicity of plain C arrays feel subjective and don't outweigh the benefits.

As an example, consider the ease at which arrays can be memset to zero using .size(), just to name something stupid (it's not stupid!). Previously, there was the risk of providing the wrong size, e.g. after refactoring or patches that change the size.

Hmm

Piece board[SQUARE_NB];
memset(board, 0, sizeof(board));

🤔

Arguments for extensibility and safety are being put forth as reasons for this patch. The SF philosophy has always been of simplicity, to add whatever is necessary but not to put stuff in "just in case". In over a decade of participating on the SF project we have not had any safety issues with C-style arrays. I'm not against changing some small subset as part of an eventual threat inputs PR but that's not a justification for a wholesale change now.
(Keep in mind the original authors and past maintainers were well aware of the tradeoffs we are now discussing and I don't think they were stupid.)

ddobbelaere · 2025-11-08T20:18:44Z

@mstembera You are right that my example wasn't the best showcase of std::array's advantages over C-style arrays. Let's please not make a religious discussion. Everyone has made his/her point, let the (current) maintainers decide!

xu-shawn · 2025-11-08T21:03:01Z

I think it's still important to stress that std::array does allow for simplicity. Take the provided example:

Piece board[SQUARE_NB];
memset(board, 0, sizeof(board));

With std::array this looks like

std::array<Piece, SQUARE_NB> board;
board.fill(Piece(0));

or even

std::array<Piece, SQUARE_NB> board{};

One could argue that C-style arrays are bearable here, but consider returning an array from a function:

Stockfish/src/position.h

Line 212 in 69a01b8

inline const Piece* Position::piece_array() const { return board; }

Now unlike std::array, the size information is completely lost. The previous memset would not work. In fact, SF later needs an awkward std::copy_n to perform a simple array copy.

Stockfish/src/nnue/nnue_accumulator.cpp

Line 416 in 69a01b8

std::copy_n(pos.piece_array(), SQUARE_NB, entry.pieces);

With std::array the syntax is much nicer.

Stockfish/src/nnue/nnue_accumulator.cpp

Line 417 in 27ed8b3

entry.pieces = pos.piece_array();

You can find more examples like this in the diff. The simplification is especially desirable for verbose memcpy calls, which can be replaced to a much simpler array assignment.

mstembera · 2025-11-08T21:14:44Z

With std::array the syntax is much nicer.

entry.pieces = pos.piece_array();

Thanks for making this point. I agree here the syntax is much nicer. However, for the vast majority of the lines this patch touches the opposite is true.

xu-shawn · 2025-11-08T21:41:50Z

I presume you are talking about the use of std::array<T, N>::data() here. There are perhaps 20 instances of it in this diff, so it is absolutely not the majority of the changes.

Moreover, is it debatable whether the use of .data is actually a case of overly complex syntax. std::array could've been designed to implicitly cast to a pointer to the underlying type, but it is not this way because conversion to a pointer is a potentially dangerous operation that is preferable to be expressed explicitly.

mstembera · 2025-11-08T22:05:49Z

Not just that. If you look at every line changed many more are more complex and verbose compared to how many are simpler.
You picked the one nice case of an array assignment to make your point but ignored just about everything else. Also there are now 358 lines versus 293. There are many places in SF one could call "potentially" dangerous but aren't and haven't been in practice. IMO this patch is a solution looking for a problem.

xu-shawn · 2025-11-08T22:10:56Z

I very much disagree with generalizing opinions only based on the line count diff. Most of the additional code is extremely localized to extending the MultiArray implementation, which has nothing to do with the main logic. A few lines of code are for extra headers. The vast majority of the diff are neither simpler or more complicated, while in quite a few places of actual program logic (not just the one I explicity demonstrated), the syntax is substantially cleaner.

mstembera · 2025-11-08T22:48:57Z

Most of the additional code is extremely localized to extending the MultiArray implementation, which has nothing to do with the main logic.

Exactly and currently this code doesn't have to exist .

A few lines of code are for extra headers.

Again currently we don't need these extra headers.

The vast majority of the diff are neither simpler or more complicated, while in quite a few places of actual program logic (not just the one I explicity demonstrated), the syntax is substantially cleaner.

Declarations and definitions are code. If I look at
uint8_t SquareDistance[SQUARE_NB][SQUARE_NB];
that's all I need to do. If I look at
MultiArray<uint8_t, SQUARE_NB, SQUARE_NB> SquareDistance;
I now need to go look at what MultiArray does.

xu-shawn · 2025-11-08T23:29:25Z

I'm afraid the matter of this disagreement has moved beyond the scope of this PR. All recent refactors of Stockfish, as seen in #5750 and #5927 are operated on the principle that simplicity is measured by expressiveness, not verbosity. This is why I do not consider the extra code in MultiArray relevant to assessing whether this PR simplifies the code. If you disagree strongly with this, then perhaps opening a discussion is a more appropriate channel.

UniQP reviewed Nov 6, 2025

View reviewed changes

vondele added the discussion label Nov 13, 2025

xu-shawn force-pushed the std_array branch 4 times, most recently from 0744762 to 316170c Compare November 14, 2025 05:56

Modernize almost all C-style array to std::array

4d5cd75

xu-shawn force-pushed the std_array branch from 316170c to 4d5cd75 Compare November 14, 2025 18:09

	std::memcpy(&indices[0], &b, sizeof(b));
	std::memcpy(indices.data(), &b, sizeof(b));

	cur = endBadCaptures = &moves[0];
	cur = endBadCaptures = moves.data();

	std::memcpy(&output[0], &biases[0], sizeof(std::int32_t) * OutputDimensions);
	std::memcpy(output, biases.data(), sizeof(std::int32_t) * OutputDimensions);

		const auto input32 = reinterpret_cast<const std::int32_t*>(&input[0]);
		const vec_t* biasvec = reinterpret_cast<const vec_t*>(&biases[0]);

	vec_t* outptr = reinterpret_cast<vec_t*>(&output[0]);
	vec_t* outptr = reinterpret_cast<vec_t*>(output);

		const auto in = reinterpret_cast<const __m256i*>(&input[0]);
		const auto out = reinterpret_cast<__m256i*>(&output[0]);

Uh oh!

Conversation

xu-shawn commented Nov 6, 2025

Uh oh!

github-actions Bot commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstembera commented Nov 6, 2025

Uh oh!

anematode commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sopel97 commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xu-shawn commented Nov 6, 2025

Uh oh!

xu-shawn commented Nov 6, 2025

Uh oh!

mstembera commented Nov 8, 2025

Uh oh!

xu-shawn commented Nov 8, 2025

Uh oh!

ddobbelaere commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstembera commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddobbelaere commented Nov 8, 2025

Uh oh!

xu-shawn commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mstembera commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xu-shawn commented Nov 8, 2025

Uh oh!

mstembera commented Nov 8, 2025

Uh oh!

xu-shawn commented Nov 8, 2025

Uh oh!

mstembera commented Nov 8, 2025

Uh oh!

xu-shawn commented Nov 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

github-actions Bot commented Nov 6, 2025 •

edited

Loading

anematode commented Nov 6, 2025 •

edited

Loading

ddobbelaere commented Nov 8, 2025 •

edited

Loading

mstembera commented Nov 8, 2025 •

edited

Loading

xu-shawn commented Nov 8, 2025 •

edited

Loading

mstembera commented Nov 8, 2025 •

edited

Loading

xu-shawn commented Nov 8, 2025 •

edited

Loading