Reorder struct lconv members to match locale.h
#6073
Conversation
`struct lconv` in locale.h https://codebrowser.dev/glibc/glibc/locale/locale.h.html#lconv::int_p_cs_precedes. Order of relevant section in glibc locale.h ```C char int_p_cs_precedes; char int_p_sep_by_space; char int_n_cs_precedes; char int_n_sep_by_space; ```
WalkthroughThe change reorders two adjacent fields, Changes
Sequence Diagram(s)Not applicable: The change only affects struct field order and does not alter control flow or introduce new features. Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNone found. Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
stdlib/src/locale.rs (1)
6-32: Add a compile-time layout assertion to guard against future regressions
Because this struct is hand-rolled and must stay byte-for-byte identical to the C version, consider adding astatic_assertions::assert_eq_size!(or similar) check against the header-generated layout on Windows. This will fail CI immediately if someone accidentally reorders or changes a field in the future.+#[cfg(windows)] +const _: () = { + use static_assertions::assert_eq_size; + // Ensure our Rust representation matches the C layout size wise. + assert_eq_size!(lconv, libc::lconv); +};(The
libccrate exposeslconvon Windows nowadays; if that ever changes, bindgen-generated bindings could be used instead.)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
stdlib/src/locale.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit Inference Engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style (cargo fmtto format)
Always run clippy to lint code (cargo clippy) before completing tasks. Fix any warnings or lints that are introduced by your changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
stdlib/src/locale.rs
🔇 Additional comments (1)
stdlib/src/locale.rs (1)
26-29:struct lconvfield order now matches C definition – good catch
The re-ordering ofint_p_sep_by_space/int_n_cs_precedesfixes the offset mismatch reported in #6072 and brings the layout in line with the C standard / glibc definition. This should eliminate the incorrect values that were observed when reading thelconvreturned bylocaleconv().
|
It doesn't affect tests but I think since C structs members are ordered in memory it technically could lead to some unusual errors. I came across this while investigating a separate issue I had where the French language locale on my laptop (Fedora OS) used a non-UTF-8 string as the thousands separator and RustPython was panicking in On MacOS my french thousands separator is On Fedora if I remember correctly (I can double check if needed) The panic would happen at RustPython/stdlib/src/locale.rs Line 177 in a9a9e3b .expect would fail if the string value in struct lconv was not utf-8 parsable. The CPython equivalent to this function accepts a wider variety of encodings without panicking. I believe CPython uses PyUnicode_DecodeUTF8Stateful to parse the string in its equivalent macro PyDict_SetItemString.
Method to check the values of >> import locale
>> locale.setlocale(locale.LC_ALL, 'fr_FR')
>> locale.localeconv()
{'int_curr_symbol': 'EUR ', 'currency_symbol': '€', 'mon_decimal_point': ',', 'mon_thousands_sep': '\u202f', 'mon_grouping': [3, 0], 'positive_sign': '', 'negative_sign': '-', 'int_frac_digits': 2, 'frac_digits': 2, 'p_cs_precedes': 0, 'p_sep_by_space': 1, 'n_cs_precedes': 0, 'n_sep_by_space': 1, 'p_sign_posn': 1, 'n_sign_posn': 1, 'decimal_point': ',', 'thousands_sep': '\u202f', 'grouping': [3, 0]}A0 is not valid UTF-8 
|
|
Thank you for explanation! |
Resolves #6072
struct lconvin locale.hhttps://codebrowser.dev/glibc/glibc/locale/locale.h.html#lconv::int_p_cs_precedes.
Order of relevant section in glibc locale.h
Summary by CodeRabbit