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

Backwards compatibility: handle 0 chunk var tiles. #4310

Merged
merged 7 commits into from
Sep 4, 2023

Conversation

KiterLuc
Copy link
Contributor

@KiterLuc KiterLuc commented Sep 1, 2023

In arrays with format version earlier than 10, it was possible to create a var file with zero chunks when all cells had an empty string value. This caused segfaults with later versions of the library and is fixed with this change.


TYPE: BUG
DESC: Backwards compatibility: handle 0 chunk var tiles.

@KiterLuc KiterLuc requested review from davisp and gspowley September 1, 2023 10:16
@shortcut-integration
Copy link

In arrays with format version earlier than 10, it was possible to create a var file with zero chunks when all cells had an empty string value. This caused segfaults with later versions of the library and is fixed with this change.

---
TYPE: BUG
DESC: Backwards compatibility: handle 0 chunk var tiles.
@KiterLuc KiterLuc force-pushed the lr/zero-chunk-var-file/ch33480 branch from 69ac408 to ce00ab9 Compare September 1, 2023 10:39
@Shelnutt2
Copy link
Member

I'm having a hard time getting the "comment on file" to stick with GitHub, so one more item.

Can we put the code used to generate the test input array as a comment somewhere in code ?This way if we need to recreate it in the future we know how it was originally produced.

@KiterLuc KiterLuc requested review from Shelnutt2 and davisp and removed request for davisp September 1, 2023 11:28
Copy link
Member

@Shelnutt2 Shelnutt2 left a comment

Choose a reason for hiding this comment

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

Thanks!

test/regression/CMakeLists.txt Outdated Show resolved Hide resolved
test/regression/targets/sc-33480.cc Outdated Show resolved Hide resolved
Copy link
Member

@ihnorton ihnorton left a comment

Choose a reason for hiding this comment

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

LGTM after the suggested cmake fix (and the note about not reproducing on M1 as discussed)

@KiterLuc KiterLuc merged commit c0e4a0e into dev Sep 4, 2023
54 checks passed
@KiterLuc KiterLuc deleted the lr/zero-chunk-var-file/ch33480 branch September 4, 2023 08:48
KiterLuc added a commit that referenced this pull request Sep 4, 2023
* Backwards compatibility: handle 0 chunk var tiles.

In arrays with format version earlier than 10, it was possible to create a var file with zero chunks when all cells had an empty string value. This caused segfaults with later versions of the library and is fixed with this change.

---
TYPE: BUG
DESC: Backwards compatibility: handle 0 chunk var tiles.

* Add unit tests.

* Adding array creation code.

* Move test to regression tests.

* Adding note for mac m1.

* Update test/regression/CMakeLists.txt

Co-authored-by: Isaiah Norton <[email protected]>

* Update test/regression/targets/sc-33480.cc

Co-authored-by: Isaiah Norton <[email protected]>

---------

Co-authored-by: Isaiah Norton <[email protected]>
(cherry picked from commit c0e4a0e)
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