Skip to content

Replaced uint8_t with unsigned char in BYTE typedef for post C99/C++ #1581

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 1, 2025
Merged

Replaced uint8_t with unsigned char in BYTE typedef for post C99/C++ #1581

merged 1 commit into from
Jun 1, 2025

Conversation

KSSAB
Copy link
Contributor

@KSSAB KSSAB commented Mar 18, 2025

Uint8_t is not necessarily a typedef for char or unsigned char (or std::byte), for this reason, it can be undefined behavior to read or modify arbitrary data through it.

Ideally, we would select uint8_t anyway if it happens to alias one of the types permitted to read or modify arbitrary data, but I don't think it can be implemented without _Generic()

The types in header are commented as not part of public api so this should not result in ABI break for those who heed the warning. There is no such warning in the .c but I would presume that is only because it is taken for granted.

@t-mat
Copy link
Contributor

t-mat commented Mar 18, 2025 •
edited
Loading

I think it's practically minor issue, since we've never reporeted any issue which is caused by (non) strict definiton of byte, octet, uint8_t and uint16_t.

But I'm also realized that how LZ4 Block Format and Frame Format is defined (and be well).

LZ4 Block Format

  • ture: "LZ4 is an LZ77-type compressor with a fixed byte-oriented encoding format."
  • ?: LZ4 Block Format depends "byte". But it doesn't specify what "byte" is. It may (not) be octet (8-bits).
  • Also the block format document implcitly expects that "consequential 2 bytes of little endian bytes = single 16-bits little endian value"

unsigned char

  • true: unsigned char is always "byte".
  • true: It is always true that sizeof(unsigned char) == 1.
  • false: CHAR_BIT in limits.h may not equal to 8.

std::byte

  • true: std::byte is always "byte".
  • true: It is always true that sizeof(std::byte) == 1.
  • false: CHAR_BIT in limits.h may not equal to 8.

uint8_t

  • true: Number of bits of uint8_t is always exactly 8. uint8_t is octet.
  • true: little endian uint8_t, uint8_t becomes little endian uint16_t

@Cyan4973 Cyan4973 self-assigned this Jun 1, 2025
Cyan4973
Cyan4973 approved these changes Jun 1, 2025
View reviewed changes
Copy link
Member

@Cyan4973 Cyan4973 left a comment

Choose a reason for hiding this comment

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

It's probably not changing anything much,
but I get the argument about type aliasing,
so I'm fine with the proposed patch.

@Cyan4973 Cyan4973 merged commit 9d04141 into lz4:dev Jun 1, 2025
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Cyan4973 Cyan4973 Cyan4973 approved these changes

Assignees

@Cyan4973 Cyan4973

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KSSAB @t-mat @Cyan4973