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

Fix wchar deserialize error when swaping the bytes #186

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

hwwei521
Copy link
Contributor

After downloading the latest Fast-CDR, I verified Fast-CDR's functionality by following the steps below:

git clone https://github.com/eProsima/Fast-CDR.git

cd Fast-CDR

mkdir build && cd build

cmake .. -DCMAKE_INSTALL_PREFIX=../install -D BUILD_TESTING=ON -D GTest_DIR=/usr/local

cmake --build . --target install

./test/cdr/UnitTests 

When I wanted to test the performance in the case of CDR swap, I changed the Cdr cdr_ser(cdrbuffer) and Cdr cdr_des(cdrbuffer) of the TEST(CDRTests, Complete) test entry to Cdr cdr_ser(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) and Cdr cdr_des(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) respectively. To make the test and the current test environment default processor Endianness inconsistent, forced to trigger the processing of CDR swap, found in EXPECT_EQ (wcscmp(c_wstring_t, c_wstring_value), 0) reported an error.

I found that swap is not considered in the Cdr& Cdr::serialize(wchar_t*& string_t) function, so I replaced it with deserialize_array(string_t, length), and it works fine.

Copy link
Contributor Author

@hwwei521 hwwei521 left a comment

Choose a reason for hiding this comment

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

This seems work fine

@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

Copy link
Contributor Author

@hwwei521 hwwei521 left a comment

Choose a reason for hiding this comment

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

@richiprosima Hi richiprosima,
Can you help review the code and allow patch submissions?
This patch will fix issues #185.

@JLBuenoLopez
Copy link
Contributor

@hwwei521

Please be patient. Currently most of eProsima developers are in holiday vacations. We will review your contribution in the next few weeks. Sorry for the wait.

@JesusPoderoso
Copy link
Contributor

Hi @hwwei521 thank you for using Fast DDS and for your contribution!

When I wanted to test the performance in the case of CDR swap, I changed the Cdr cdr_ser(cdrbuffer) and Cdr cdr_des(cdrbuffer) of the TEST(CDRTests, Complete) test entry to Cdr cdr_ser(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) and Cdr cdr_des(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) respectively. To make the test and the current test environment default processor Endianness inconsistent, forced to trigger the processing of CDR swap, found in EXPECT_EQ (wcscmp(c_wstring_t, c_wstring_value), 0) reported an error.

Could you include another commit with a regression test including the way you test it (as you have described above) to keep track of it?

@hwwei521
Copy link
Contributor Author

Hi @hwwei521 thank you for using Fast DDS and for your contribution!

When I wanted to test the performance in the case of CDR swap, I changed the Cdr cdr_ser(cdrbuffer) and Cdr cdr_des(cdrbuffer) of the TEST(CDRTests, Complete) test entry to Cdr cdr_ser(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) and Cdr cdr_des(cdrbuffer, eprosima::fastcdr::Cdr::Endianness::BIG_ENDIANNESS) respectively. To make the test and the current test environment default processor Endianness inconsistent, forced to trigger the processing of CDR swap, found in EXPECT_EQ (wcscmp(c_wstring_t, c_wstring_value), 0) reported an error.

Could you include another commit with a regression test including the way you test it (as you have described above) to keep track of it?

Hi, Jesús
You want me to submit a patch of the program I was testing at the time to do a regression test right?
I was doing the test as below, and if I need to submit a patch for regression testing, I'll submit another patch later for regression test validation use.

git clone https://github.com/eProsima/Fast-CDR.git

cd Fast-CDR

mkdir build && cd build

cmake .. -DCMAKE_INSTALL_PREFIX=../install -D BUILD_TESTING=ON -D GTest_DIR=/usr/local

cmake --build . --target install

./test/cdr/UnitTests 

@JesusPoderoso
Copy link
Contributor

Hi Hanwuwei, I meant including a test like this one in the test suite, where we can try to run with and without your fix to check that is solves it.

@hwwei521
Copy link
Contributor Author

hwwei521 commented Jan 15, 2024

Hi Hanwuwei, I meant including a test like this one in the test suite, where we can try to run with and without your fix to check that is solves it.

Hi, Jesús
I update another patch #189, Please help to check.
#189
The test result as the file show
UnitTests_reports.txt

@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@hwwei521
Copy link
Contributor Author

@JesusPoderoso
Hi, Jesús, The simple regression test patch added in #190

weihanwu and others added 2 commits February 20, 2024 12:50
Signed-off-by: weihanwu <weihanwu@lixiang.com>
Signed-off-by: weihanwu <weihanwu@lixiang.com>
Co-authored-by: maxin <maxin@lixiang.com>
@MiguelCompany
Copy link
Member

I rebased this on top of master, and included the test from #190

test/cdr/SimpleTest.cpp Outdated Show resolved Hide resolved
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Co-authored-by: Ricardo González <correoricky@gmail.com>
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Mac Build Status
  • Windows Build Status

@MiguelCompany
Copy link
Member

Going in. Thank you @hwwei521 for your contribution!

@MiguelCompany MiguelCompany merged commit 3b94bdb into eProsima:master Feb 21, 2024
8 of 12 checks passed
EduPonz pushed a commit that referenced this pull request Feb 22, 2024
* Regression test for #185 in SimpleTest

Signed-off-by: weihanwu <weihanwu@lixiang.com>

* Fix wchar deserialize error when swaping the bytes

Signed-off-by: weihanwu <weihanwu@lixiang.com>
Co-authored-by: maxin <maxin@lixiang.com>

* Apply suggestion

Signed-off-by: Miguel Company <miguelcompany@eprosima.com>

Co-authored-by: Ricardo González <correoricky@gmail.com>

---------

Signed-off-by: weihanwu <weihanwu@lixiang.com>
Co-authored-by: maxin <maxin@lixiang.com>
Co-authored-by: Miguel Company <miguelcompany@eprosima.com>
Co-authored-by: Ricardo González <correoricky@gmail.com>
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.

wchar deserialize error when swaping the bytes
7 participants