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

Add support of multi-dim C-style array member of struct. #4262

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

peng-wang-cn
Copy link

  • Support up to 4 dimensional array.

  • Modify pretty-print logic to print multi-dim array's inner most dimension in the same line for better readability.

Refer to discussion #4248 .

@coveralls
Copy link

coveralls commented Jan 6, 2024

Coverage Status

coverage: 100.0%. remained the same
when pulling 68c0097 on peng-wang-cn:multidim-array
into a259ecc on nlohmann:develop.

@peng-wang-cn
Copy link
Author

It looks like the failed tests are due to a clang bug #75943, which was fixed by PR #76007, 2 weeks ago.
So @nlohmann , what do we do with this?

}

SECTION("indent=4")
{
CHECK(j.dump(4) ==
"{\n \"array\": [\n 1,\n 2,\n 3,\n 4\n ],\n \"boolean\": false,\n \"null\": null,\n \"number\": 42,\n \"object\": {},\n \"string\": \"Hello world\"\n}");
Copy link
Owner

Choose a reason for hiding this comment

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

You changed the previous dump output here. Please revert this.

Copy link
Author

Choose a reason for hiding this comment

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

If the one-line array print is going to be accepted, existing tests for array print checks must also be changed. Honestly I am also afraid that changing default array print formats will incur backward compatibility issues. Maybe we should use a switch to control the one-line array format? By default the switch is off so that existing formats are maintained.

Copy link
Owner

Choose a reason for hiding this comment

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

The thing is: another switch to dump() won't be helping - it already has too many options. We rather need an approach for a second overload of dump() that takes a proper configuration. For now, I would rather split the support for multi-dimensional arrays (this PR) and their formatting (later PR).

Copy link
Author

Choose a reason for hiding this comment

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

OK. I'll remove the dump formatting stuff from this PR. Thanks for the suggestion.

@@ -193,7 +193,10 @@ class serializer
return;
}

if (pretty_print)
// For multi-dim number arrays, print the inner most dimension in one line to improve readability.
Copy link
Owner

Choose a reason for hiding this comment

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

I think the comment is misleading: you also change the behavior for 1-dimensional arrays.

@nlohmann
Copy link
Owner

nlohmann commented Jan 6, 2024

It looks like the failed tests are due to a clang bug #75943, which was fixed by PR #76007, 2 weeks ago. So @nlohmann , what do we do with this?

Please fix such that there is no error with Clangs out there that still have that bug.

* Support up to 4 dimensional array.
@peng-wang-cn
Copy link
Author

peng-wang-cn commented Jan 9, 2024

It looks like the failed tests are due to a clang bug #75943, which was fixed by PR #76007, 2 weeks ago. So @nlohmann , what do we do with this?

Please fix such that there is no error with Clangs out there that still have that bug.

Using docker image tuxmake/clang-nightly:20240109, ci_test_clang will pass. Can we update the CI config to use images from tuxmake/clang-nightly? @nlohmann

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants