Skip to content

Conversation

@khwilliamson
Copy link
Contributor

These are claimed to exist in our documentation, but didn't actually until this commit.

  • This set of changes does not require a perldelta entry.

These are claimed to exist in our documentation, but didn't actually
until this commit.
@Leont
Copy link
Contributor

Leont commented Sep 18, 2025

Do we really need them? We already mostly require C99, are these macros really missing on platforms?

Copy link

@Imran-imtiaz48 Imran-imtiaz48 left a comment

Choose a reason for hiding this comment

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

The changes you introduced around the integer constant macros look good overall and follow the expected pattern for defining INTxx_C and UINTxx_C. I appreciate the effort to make the definitions conditional on INTSIZE, which ensures portability across different platforms. One thing to note is consistency: the suffix usage (L for signed, U for unsigned) is correct, but in the UINT16_C branch you directly append U without considering L for longer integer cases, which might cause issues on compilers where int is smaller than 16 bits. It would be worth double-checking whether appending UL is safer in environments where unsigned int is not guaranteed to hold 16 bits. Additionally, the formatting looks a little uneven, so aligning indentation across all the macro definitions would improve readability. Overall, this is a solid improvement for type safety and cross-platform compatibility, but I recommend a quick pass for suffix consistency and style polish before merging.

@Leont
Copy link
Contributor

Leont commented Sep 18, 2025

I think these definitions are unnecessarily defensive. I'm not sure if we ever supported architectures where sizeof(int) < 4, but if we did we dropped support for it a very long time ago.

@tonycoz
Copy link
Contributor

tonycoz commented Sep 22, 2025

These are claimed to exist in our documentation, but didn't actually until this commit.

They're only documented because of ca336c5

But I also think it's better to just allow the standard headers to define them.

@khwilliamson
Copy link
Contributor Author

Then, shouldn't we remove the ones we already have as in #23753

@khwilliamson
Copy link
Contributor Author

#23753 has been merged, instead of this

@khwilliamson khwilliamson deleted the INT16_C branch September 25, 2025 00:39
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