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 warnings in BinarySerializer #963

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erer1243
Copy link
Contributor

This PR adjusts BinarySerializer in these ways:

  • make m_buffer not const (we write to it, why was it ever const)
  • unaligned reads/writes are done correctly, using std::memcpy
  • signedness cast warnings are fixed

A short discussion on this was held here previously: #921 (comment)

On x86/ARM, unaligned reads/writes are acceptable with general purpose registers, but they may crash on x86 if the optimizer decides to use a SIMD register.

@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@qiluo-msft qiluo-msft requested a review from liuh-80 January 2, 2025 23:26
@@ -34,7 +43,7 @@ class BinarySerializer {
}

static size_t serializeBuffer(
const char* buffer,
char* buffer,
Copy link

Choose a reason for hiding this comment

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

Just curious why remove const here? Are we modifying the buffer?

Copy link
Contributor Author

@erer1243 erer1243 Jan 3, 2025

Choose a reason for hiding this comment

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

Yes, the function serializes kcos into buffer. It should never have been const.

@erer1243 erer1243 force-pushed the fix-binaryserializer branch from 2dedd07 to e5f0948 Compare January 15, 2025 18:54
@mssonicbld
Copy link
Collaborator

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

4 participants