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

Merged
merged 3 commits into from
Dec 10, 2024

Conversation

peng-wang-cn
Copy link
Contributor

  • 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: 99.65% (+0.001%) from 99.649%
when pulling d8248e7 on peng-wang-cn:multidim-array
into 9f60e85 on nlohmann:develop.

@peng-wang-cn
Copy link
Contributor 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?

tests/src/unit-inspection.cpp Outdated Show resolved Hide resolved
include/nlohmann/detail/output/serializer.hpp Outdated Show resolved Hide resolved
@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
Contributor 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

@nlohmann
Copy link
Owner

Can you update to the latest develop branch - this should fix the CI - especially Clang-Tidy.

@nlohmann nlohmann added the please rebase Please rebase your branch to origin/develop label Nov 20, 2024
@nlohmann nlohmann removed the please rebase Please rebase your branch to origin/develop label Dec 9, 2024
@peng-wang-cn
Copy link
Contributor Author

Can you update to the latest develop branch - this should fix the CI - especially Clang-Tidy.

Now the only remaining CI errors are:

FAILED: tests/CMakeFiles/test-conversions_cpp11.dir/src/unit-conversions.cpp.o 
/__w/_temp/447077027/cmake-3.31.0-linux-x86_64/bin/cmake -E __run_co_compile --tidy="/usr/bin/clang-tidy-20;--extra-arg-before=--driver-mode=g++" --source=/__w/json/json/tests/src/unit-conversions.cpp -- /usr/bin/clang++ -DDOCTEST_CONFIG_SUPER_FAST_ASSERTS -DJSON_TEST_KEEP_MACROS -DJSON_TEST_USING_MULTIPLE_HEADERS=1 -I/__w/json/json/tests/thirdparty/doctest -I/__w/json/json/tests/thirdparty/fifo_map -I/__w/json/json/build/build_clang_tidy/include -I/__w/json/json/include -g -std=gnu++11 -Wno-deprecated -Wno-float-equal -MD -MT tests/CMakeFiles/test-conversions_cpp11.dir/src/unit-conversions.cpp.o -MF tests/CMakeFiles/test-conversions_cpp11.dir/src/unit-conversions.cpp.o.d -o tests/CMakeFiles/test-conversions_cpp11.dir/src/unit-conversions.cpp.o -c /__w/json/json/tests/src/unit-conversions.cpp
/__w/json/json/tests/src/unit-conversions.cpp:384:19: error: do not declare C-style arrays, use 'std::array' instead [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays,-warnings-as-errors]
  384 |             const int nbs[][2][3] = {\
      |                   ^
/__w/json/json/tests/src/unit-conversions.cpp:388:13: error: do not declare C-style arrays, use 'std::array' instead [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays,-warnings-as-errors]
  388 |             int nbs2[][2][3] = {\
      |             ^
/__w/json/json/tests/src/unit-conversions.cpp:400:19: error: do not declare C-style arrays, use 'std::array' instead [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays,-warnings-as-errors]
  400 |             const int nbs[][2][2][3] = {\
      |                   ^
/__w/json/json/tests/src/unit-conversions.cpp:412:13: error: do not declare C-style arrays, use 'std::array' instead [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays,-warnings-as-errors]
  412 |             int nbs2[][2][2][3] = {\
      |             ^
90313 warnings generated.
Suppressed 90618 warnings (90271 in non-user code, 347 NOLINT).
Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
4 warnings treated as errors

I'm not sure how to proceed. The purpose of this change is to support C-style arrays, which contradicts the clang-tidy rules of avoiding them. What is your suggestion? @nlohmann

@nlohmann
Copy link
Owner

You can add NOLINT comments for them.

Copy link
Owner

@nlohmann nlohmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@nlohmann nlohmann added this to the Release 3.11.4 milestone Dec 10, 2024
@nlohmann nlohmann merged commit 589641b into nlohmann:develop Dec 10, 2024
122 checks passed
@nlohmann
Copy link
Owner

Thanks!

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