Skip to content

Conversation

@YongDo-Hyun
Copy link

This PR focuses on correctness fixes, test coverage improvements, and internal refactoring.

Changes

  1. Fixed JSON string output to properly escape special and control characters.
  2. Refactored numeric value assignments to remove duplicated logic and prevent unintended truncation when assigning wider types.
  3. Improved exception safety in ozlibstream::close() by avoiding exception masking.
  4. Added additional tests:
  • File read and write round trip test for formatted output.
  • Optional local developer test executable for verifying numeric value assignments.
  1. Updated README with build instructions and prerequisites.
  2. Updated .gitignore to ignore local editor configuration.

Motivation

While working with libnbtplusplus in real world usage, I encountered several edge cases:

  1. JSON output produced invalid strings for control characters.
  2. Assigning wider numeric types to existing value instances could silently truncate data.
  3. Some stream error paths could replace the original exception.

These changes aim to make the library safer and more predictable without altering its public API.

Notes

  1. The local test executable is optional and gated behind NBT_BUILD_LOCAL_TEST.
  2. No behavioral changes are expected for existing correct usage.

Signed-off-by: YongDo-Hyun froster12@naver.com

- Added option to build a local test executable for value assignments.
- Enhanced JSON string formatting by escaping special characters.
- Updated README with build instructions and prerequisites.
- Modified .gitignore to include .vscode directory.
- Added file read/write tests in format_test.cpp.
- Refactored value assignment logic to reduce code duplication.

Signed-off-by: YongDo-Hyun <froster12@naver.com>
@YongDo-Hyun YongDo-Hyun requested a review from Trial97 December 26, 2025 15:51
@YongDo-Hyun
Copy link
Author

YongDo-Hyun commented Dec 26, 2025

Thanks for review @Trial97 build and test results in here thanks.

samet@192: /src/github/orgs/Project-Tick/libnbtplusplus$ cmake -S . -B build -G Ninja
-- The C compiler identification is GNU 15.2.1
-- The CXX compiler identification is GNU 15.2.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/lib64/ccache/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/lib64/ccache/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ZLIB: /usr/lib64/libz.so (found version "1.3.1")
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY - Success
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY - Success
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Success
-- Found CxxTest: /usr/include
-- Configuring done (0.6s)
-- Generating done (0.0s)
-- Build files have been written to: /home/samet/src/github/orgs/Project-Tick/libnbtplusplus/build
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus$ cmake --build build
[45/45] Linking CXX executable test/nbttest
samet@192:~/src/github/orgs/Project-Tick/libnbtplusplus$ ctest --test-dir build --output-on-failure
Internal ctest changing into directory: /home/samet/src/github/orgs/Project-Tick/libnbtplusplus/build
Test project /home/samet/src/github/orgs/Project-Tick/libnbtplusplus/build
Start 1: nbttest
1/7 Test 1: nbttest .......................... Passed 0.00 sec
Start 2: endian_str_test
2/7 Test 2: endian_str_test .................. Passed 0.00 sec
Start 3: read_test
3/7 Test 3: read_test ........................ Passed 0.00 sec
Start 4: write_test
4/7 Test 4: write_test ....................... Passed 0.00 sec
Start 5: zlibstream_test
5/7 Test 5: zlibstream_test .................. Passed 0.00 sec
Start 6: format_test
6/7 Test 6: format_test ...................... Passed 0.00 sec
Start 7: test_value
7/7 Test 7: test_value ....................... Passed 0.00 sec

100% tests passed, 0 tests failed out of 7

Total Test time (real) = 0.03 sec
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus$

@Trial97 Trial97 removed their request for review December 26, 2025 16:22
@Ryex
Copy link
Member

Ryex commented Dec 26, 2025

why is this pr touching half the code base with code style breaking formatting? I can't even tell what code is fixing the json anymore.

Signed-off-by: YongDo-Hyun <froster12@naver.com>
… in json_formatter; add make_numeric_tag for cleaner tag creation

Signed-off-by: YongDo-Hyun <froster12@naver.com>
@YongDo-Hyun
Copy link
Author

YongDo-Hyun commented Dec 26, 2025

why is this pr touching half the code base with code style breaking formatting? I can't even tell what code is fixing the json anymore.

Thanks for reviewing @Ryex i resolved this issue and im sorry for this. thanks.

Test results:

samet@192: /src/github/orgs/Project-Tick/libnbtplusplus$ cmake -S . -B build -G Ninja
-- The C compiler identification is GNU 15.2.1
-- The CXX compiler identification is GNU 15.2.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/lib64/ccache/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/lib64/ccache/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ZLIB: /usr/lib64/libz.so (found version "1.3.1")
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY - Success
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY - Success
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Success
-- Found CxxTest: /usr/include
-- Configuring done (0.7s)
-- Generating done (0.0s)
-- Build files have been written to: /home/samet/src/github/orgs/Project-Tick/libnbtplusplus/build
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus$ cd build
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus/build$ cmake --build .
[45/45] Linking CXX executable test/write_test
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus/build$ ctest --output-on-failure
Test project /home/samet/src/github/orgs/Project-Tick/libnbtplusplus/build
Start 1: nbttest
1/7 Test 1: nbttest .......................... Passed 0.01 sec
Start 2: endian_str_test
2/7 Test 2: endian_str_test .................. Passed 0.00 sec
Start 3: read_test
3/7 Test 3: read_test ........................ Passed 0.01 sec
Start 4: write_test
4/7 Test 4: write_test ....................... Passed 0.00 sec
Start 5: zlibstream_test
5/7 Test 5: zlibstream_test .................. Passed 0.00 sec
Start 6: format_test
6/7 Test 6: format_test ...................... Passed 0.00 sec
Start 7: test_value
7/7 Test 7: test_value ....................... Passed 0.00 sec

100% tests passed, 0 tests failed out of 7

Total Test time (real) = 0.03 sec
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus/build$

…test output handling

Signed-off-by: YongDo-Hyun <froster12@naver.com>
Signed-off-by: YongDo-Hyun <froster12@naver.com>
…lue assignments

Signed-off-by: YongDo-Hyun <froster12@naver.com>
Signed-off-by: YongDo-Hyun <froster12@naver.com>
@YongDo-Hyun
Copy link
Author

samet@192: /src/github/orgs/Project-Tick/libnbtplusplus$ cmake -S . -B build -G Ninja
-- The C compiler identification is GNU 15.2.1
-- The CXX compiler identification is GNU 15.2.1
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/lib64/ccache/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /usr/lib64/ccache/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ZLIB: /usr/lib64/libz.so (found version "1.3.1")
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY - Success
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY - Success
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Success
-- Found CxxTest: /usr/include
-- Configuring done (0.6s)
-- Generating done (0.0s)
-- Build files have been written to: /home/samet/src/github/orgs/Project-Tick/libnbtplusplus/build
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus$ cmake --build build
[46/46] Linking CXX executable test/nbttest
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus$ cd build
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus/build$ ctest --output-on-failure
Test project /home/samet/src/github/orgs/Project-Tick/libnbtplusplus/build
Start 1: nbttest
1/7 Test 1: nbttest .......................... Passed 0.00 sec
Start 2: endian_str_test
2/7 Test 2: endian_str_test .................. Passed 0.00 sec
Start 3: read_test
3/7 Test 3: read_test ........................ Passed 0.00 sec
Start 4: write_test
4/7 Test 4: write_test ....................... Passed 0.00 sec
Start 5: zlibstream_test
5/7 Test 5: zlibstream_test .................. Passed 0.00 sec
Start 6: format_test
6/7 Test 6: format_test ...................... Passed 0.00 sec
Start 7: test_value
7/7 Test 7: test_value ....................... Passed 0.00 sec

100% tests passed, 0 tests failed out of 7

Total Test time (real) = 0.01 sec
samet@192: /src/github/orgs/Project-Tick/libnbtplusplus/build$

…ing errors

Signed-off-by: YongDo-Hyun <froster12@naver.com>
@YongDo-Hyun YongDo-Hyun requested a review from Trial97 December 27, 2025 10:49
Signed-off-by: YongDo-Hyun <froster12@naver.com>
…nt masking errors

Signed-off-by: YongDo-Hyun <froster12@naver.com>
Signed-off-by: YongDo-Hyun <froster12@naver.com>
@YongDo-Hyun
Copy link
Author

YongDo-Hyun commented Dec 27, 2025

~/libnbtplusplus contributeforprism ❯ cmake -S . -B build -G Ninja -DZLIB_ROOT=/nix/store/hqvsiah013yzb17b13fn18fpqk7m13cg-zlib-1.3.1-dev -DCXXTEST_INCLUDE_DIR=/nix/store/wvd95y7qi7gg7a2g506i0cfp6h87980x-cxxtest-4.4
-- The C compiler identification is GNU 14.3.0
-- The CXX compiler identification is GNU 14.3.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /run/current-system/sw/bin/cc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /run/current-system/sw/bin/c++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Found ZLIB: /run/current-system/sw/lib/libz.so (found version "1.3.1")
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_VISIBILITY - Success
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY
-- Performing Test COMPILER_HAS_HIDDEN_INLINE_VISIBILITY - Success
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR
-- Performing Test COMPILER_HAS_DEPRECATED_ATTR - Success
-- Found CxxTest: /nix/store/wvd95y7qi7gg7a2g506i0cfp6h87980x-cxxtest-4.4
-- Configuring done (0.9s)
-- Generating done (0.0s)
-- Build files have been written to: /home/samet/libnbtplusplus/build
~/libnbtplusplus contributeforprism ❯ LC_ALL=C nix-shell -p cxxtest python3 --run "cmake --build build"
[40/46] Linking CXX executable test/zlibstream_test
/nix/store/dc9vaz50jg7mibk9xvqw5dqv89cxzla3-binutils-2.44/bin/ld: warning: trailing_data.zlib.obj: missing .note.GNU-stack section implies executable stack
/nix/store/dc9vaz50jg7mibk9xvqw5dqv89cxzla3-binutils-2.44/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
[42/46] Linking CXX executable test/write_test
/nix/store/dc9vaz50jg7mibk9xvqw5dqv89cxzla3-binutils-2.44/bin/ld: warning: bigtest_uncompr.obj: missing .note.GNU-stack section implies executable stack
/nix/store/dc9vaz50jg7mibk9xvqw5dqv89cxzla3-binutils-2.44/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
[44/46] Linking CXX executable test/read_test
/nix/store/dc9vaz50jg7mibk9xvqw5dqv89cxzla3-binutils-2.44/bin/ld: warning: toplevel_string.obj: missing .note.GNU-stack section implies executable stack
/nix/store/dc9vaz50jg7mibk9xvqw5dqv89cxzla3-binutils-2.44/bin/ld: NOTE: This behaviour is deprecated and will be removed in a future version of the linker
[46/46] Linking CXX executable test/nbttest
~/libnbtplusplus contributeforprism 6s ❯ cd build
~/libnbtplusplus/build contributeforprism ❯ ctest --output-on-failure
Test project /home/samet/libnbtplusplus/build
Start 1: nbttest
1/7 Test 1: nbttest .......................... Passed 0.00 sec
Start 2: endian_str_test
2/7 Test 2: endian_str_test .................. Passed 0.00 sec
Start 3: read_test
3/7 Test 3: read_test ........................ Passed 0.00 sec
Start 4: write_test
4/7 Test 4: write_test ....................... Passed 0.00 sec
Start 5: zlibstream_test
5/7 Test 5: zlibstream_test .................. Passed 0.00 sec
Start 6: format_test
6/7 Test 6: format_test ...................... Passed 0.00 sec
Start 7: test_value
7/7 Test 7: test_value ....................... Passed 0.00 sec

100% tests passed, 0 tests failed out of 7

Total Test time (real) = 0.02 sec
~/libnbtplusplus/build contributeforprism ❯

@YongDo-Hyun YongDo-Hyun requested a review from Ryex December 27, 2025 14:22
@Ryex
Copy link
Member

Ryex commented Dec 27, 2025

looking much better now. the only thing I'd ask (though not nessacery if we squash merge) is a clean up of the commit history to be more atomic.

thanks for working with us.

@YongDo-Hyun
Copy link
Author

YongDo-Hyun commented Dec 27, 2025

looking much better now. the only thing I'd ask (though not nessacery if we squash merge) is a clean up of the commit history to be more atomic.

thanks for working with us.

Thanks for reviewing @Ryex. I think not need for this because its only 11 commit. and all tests result is correct

Co-authored-by: Alexandru Ionut Tripon <alexandru.tripon97@gmail.com>
Signed-off-by: YongDo-Hyun <97219311+YongDo-Hyun@users.noreply.github.com>
YongDo-Hyun and others added 3 commits December 28, 2025 00:09
Co-authored-by: Alexandru Ionut Tripon <alexandru.tripon97@gmail.com>
Signed-off-by: YongDo-Hyun <97219311+YongDo-Hyun@users.noreply.github.com>
Co-authored-by: Alexandru Ionut Tripon <alexandru.tripon97@gmail.com>
Signed-off-by: YongDo-Hyun <97219311+YongDo-Hyun@users.noreply.github.com>
Co-authored-by: Alexandru Ionut Tripon <alexandru.tripon97@gmail.com>
Signed-off-by: YongDo-Hyun <97219311+YongDo-Hyun@users.noreply.github.com>
@YongDo-Hyun
Copy link
Author

YongDo-Hyun commented Dec 27, 2025

i think all done thanks @Trial97. @Ryex thanks for open discussion on my repo thanks lastly im porting it myself as a theme thanks.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants