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

[C++] Update to address comparator failure in big endian #7681

Merged
merged 28 commits into from
Dec 6, 2022

Conversation

sunwen18
Copy link
Contributor

@sunwen18 sunwen18 commented Nov 30, 2022

Address issue #7685 of KeyCompareWithValue fail on big endian machines
The previous implementation compares an element from a array[index] and an element from Array.Get[index], the Get function performs and extra EndianSwap, thus the comparison is wrong.

  • Update the idl_gen_cpp.cpp to address the issue
  • Update unit test

Current implementation tested by running a docker image and pull the PR down and run. Following instructions here: https://til.simonwillison.net/docker/emulate-s390x-with-qemu

@github-actions github-actions bot added the codegen Involving generating code from schema label Dec 2, 2022
@sunwen18 sunwen18 changed the title Test endianswap for key_field_test Update to address comparator failure in big endian Dec 2, 2022
@sunwen18 sunwen18 marked this pull request as ready for review December 2, 2022 17:27
@github-actions github-actions bot added the documentation Documentation label Dec 3, 2022
@github-actions github-actions bot removed the documentation Documentation label Dec 3, 2022
@sunwen18 sunwen18 changed the title Update to address comparator failure in big endian [C++] Update to address comparator failure in big endian Dec 3, 2022
src/idl_gen_cpp.cpp Outdated Show resolved Hide resolved
tests/key_field_test.cpp Outdated Show resolved Hide resolved
@sunwen18 sunwen18 requested a review from dbaileychess December 5, 2022 18:24
tests/key_field/key_field_sample_generated.h Outdated Show resolved Hide resolved
tests/key_field/key_field_sample_generated.h Outdated Show resolved Hide resolved
tests/key_field/key_field_sample_generated.h Outdated Show resolved Hide resolved
dbaileychess
dbaileychess previously approved these changes Dec 6, 2022
tests/key_field/key_field_sample_generated.h Outdated Show resolved Hide resolved
dbaileychess
dbaileychess previously approved these changes Dec 6, 2022
@dbaileychess
Copy link
Collaborator

Thanks @sunwen18 Did you confirm that this fixes the issue on the Big Endian system?

@sunwen18
Copy link
Contributor Author

sunwen18 commented Dec 6, 2022

Thanks @sunwen18 Did you confirm that this fixes the issue on the Big Endian system?

Yes, I followed the link you found and built a docker locally and then pull my fix and then run unit test, it can pass.

@sunwen18 sunwen18 requested a review from dbaileychess December 6, 2022 05:02
@dbaileychess dbaileychess merged commit b5ebd3f into google:master Dec 6, 2022
candhyan pushed a commit to mediaz/flatbuffers that referenced this pull request Jan 2, 2023
* update unit test and generated file to test is extra endianswap can help resolve issue

* remove EndianScalar wrapper from Get method

* remove endianscalar wrapper

* update

* update

* use Array instead

* clang format

* address error

* clang

* update

* manually generate

* Move Nim to completed language

* Add swift link

* address comments

* update unit test

* address comment

* address comment

* regenerate file

* use auto instead of size_t

* use uint32_t instead

* update

* format

* delete extra whitespace

Co-authored-by: Wen Sun <sunwen@google.com>
Co-authored-by: Derek Bailey <derekbailey@google.com>
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
* update unit test and generated file to test is extra endianswap can help resolve issue

* remove EndianScalar wrapper from Get method

* remove endianscalar wrapper

* update

* update

* use Array instead

* clang format

* address error

* clang

* update

* manually generate

* Move Nim to completed language

* Add swift link

* address comments

* update unit test

* address comment

* address comment

* regenerate file

* use auto instead of size_t

* use uint32_t instead

* update

* format

* delete extra whitespace

Co-authored-by: Wen Sun <sunwen@google.com>
Co-authored-by: Derek Bailey <derekbailey@google.com>
jochenparm pushed a commit to jochenparm/flatbuffers that referenced this pull request Oct 29, 2024
* update unit test and generated file to test is extra endianswap can help resolve issue

* remove EndianScalar wrapper from Get method

* remove endianscalar wrapper

* update

* update

* use Array instead

* clang format

* address error

* clang

* update

* manually generate

* Move Nim to completed language

* Add swift link

* address comments

* update unit test

* address comment

* address comment

* regenerate file

* use auto instead of size_t

* use uint32_t instead

* update

* format

* delete extra whitespace

Co-authored-by: Wen Sun <sunwen@google.com>
Co-authored-by: Derek Bailey <derekbailey@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ codegen Involving generating code from schema
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants