Skip to content

Add localeconv function to locale module #4558

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

Merged
youknowone merged 30 commits into from
Feb 28, 2023
+169 −4
Merged

Add localeconv function to locale module #4558

Changes from 1 commit
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
b3e39a2
Add localeconv function to locale module
minhrongcon2000 Feb 24, 2023
1ed1b29
Fix potential infinite loop
minhrongcon2000 Feb 24, 2023
7e9390c
Skip locale test
minhrongcon2000 Feb 24, 2023
b2c1e5f
Refactor and add platform def for localeconv
minhrongcon2000 Feb 24, 2023
0dd95e0
Add missing platform def on constant
minhrongcon2000 Feb 24, 2023
65f5036
Fix wrong format
minhrongcon2000 Feb 24, 2023
359c696
Fix platform dependent error build
minhrongcon2000 Feb 24, 2023
1533f7b
Add platform def at the top of locale module
minhrongcon2000 Feb 24, 2023
09c750c
Refactor code
minhrongcon2000 Feb 24, 2023
844a30a
Fix mismatch typing
minhrongcon2000 Feb 24, 2023
26f103a
Use libc::c_char instead
minhrongcon2000 Feb 24, 2023
9374005
Fix skip test reason for locale
minhrongcon2000 Feb 24, 2023
757545f
Add setlocale function
minhrongcon2000 Feb 24, 2023
184f891
Merge commit '757545fe37c347c75a5734e0fa9ef9d13a3bcce8' into fix/add-…
minhrongcon2000 Feb 25, 2023
ecc21e8
Fix clippy requirements
minhrongcon2000 Feb 25, 2023
973d2b2
Fix rustfmt issues
minhrongcon2000 Feb 25, 2023
2d37933
Remove test_locale skip testcase
minhrongcon2000 Feb 25, 2023
082d1ef
Fix unittest
minhrongcon2000 Feb 25, 2023
f5730c4
Fix clippy issues
minhrongcon2000 Feb 25, 2023
31941a4
Clean up locale module code
minhrongcon2000 Feb 25, 2023
0c5ecb5
Remove setlocale test expected failure
minhrongcon2000 Feb 25, 2023
26f04e2
Skip format and strcol test case
minhrongcon2000 Feb 28, 2023
0268af5
Fix skpTest bug
minhrongcon2000 Feb 28, 2023
44a36af
Skip locale test for window
minhrongcon2000 Feb 28, 2023
9801409
Fix test locale skip convention
minhrongcon2000 Feb 28, 2023
fcfa35a
Revert original skip decorator and polish code
minhrongcon2000 Feb 28, 2023
a44a593
Allow test_strcoll_3303
minhrongcon2000 Feb 28, 2023
9e5be11
Expect failure instead of skip
minhrongcon2000 Feb 28, 2023
6291ca9
Update Lib/test/test_locale.py
youknowone Feb 28, 2023
38d2253
clean up and additional error handling for NulError
youknowone Feb 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Clean up locale module code
  • Loading branch information
@minhrongcon2000
minhrongcon2000 committed Feb 25, 2023
commit 31941a46b7336a43253d5182748f6a167f30ca19
31 changes: 11 additions & 20 deletions stdlib/src/locale.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ mod _locale {
T_FMT_AMPM, YESEXPR,
};
Comment on lines +18 to +27
Copy link
Copy Markdown
Contributor

@fanninpm fanninpm Feb 24, 2023

Choose a reason for hiding this comment

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

Not all of these constants are available on every platform. When I encountered this sort of problem in the past, I looked up each item individually in the libc repository to see which platforms support that item. (See #3840, for example.)

I realized back then (and I still maintain) that this process would prove to be tedious and error-prone. Perhaps I should look into using the autocfg crate instead, but that's ultimately beyond the scope of this PR.

Copy link
Copy Markdown
Contributor Author

@minhrongcon2000 minhrongcon2000 Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I tried with Window locale.h this afternoon and found out that Window API uses different naming convention (prefix with W, if I remember). Hence, if we would like to fix it, I'm afraid that we need to implement our own locale interaction. I will do it in another PR once this module takes shape.

Right now, I put this module limit to unix system only!


use std::{ffi::CStr, ptr};
use std::{
ffi::{CStr, CString},
ptr,
};

#[pyattr(name = "CHAR_MAX")]
fn char_max(vm: &VirtualMachine) -> PyIntRef {
Expand All @@ -44,7 +47,7 @@ mod _locale {
vm.ctx.new_list(group_vec)
}

unsafe fn _parse_ptr_to_str(vm: &VirtualMachine, raw_ptr: *mut libc::c_char) -> PyResult {
unsafe fn _parse_ptr_to_str(vm: &VirtualMachine, raw_ptr: *const libc::c_char) -> PyResult {
let slice = unsafe { CStr::from_ptr(raw_ptr) };
let cstr = slice
.to_str()
Expand Down Expand Up @@ -124,25 +127,13 @@ mod _locale {
#[pyfunction]
fn setlocale(args: LocaleArgs, vm: &VirtualMachine) -> PyResult {
unsafe {
let result = match args.locale {
OptionalArg::Missing => {
let null_ptr: *const i8 = ptr::null();
libc::setlocale(args.category, null_ptr)
let result = match args.locale.flatten() {
None => libc::setlocale(args.category, ptr::null()),
Some(l) => {
let l_str = CString::new(l.to_string()).expect("expect to be always converted");
Copy link
Copy Markdown
Member

@youknowone youknowone Feb 28, 2023

Choose a reason for hiding this comment

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

this is not always successful

>>> _locale.setlocale(0, '\0')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: embedded null character

let l_ptr = CStr::as_ptr(&l_str);
Copy link
Copy Markdown
Member

@youknowone youknowone Feb 28, 2023

Choose a reason for hiding this comment

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

because CString implements std::ops::Deref<Target=CStr>, this is redundant.

libc::setlocale(args.category, lstr.as_ptr())

works exactly same way.

libc::setlocale(args.category, l_ptr)
}
OptionalArg::Present(locale) => match locale {
None => {
let null_ptr: *const i8 = ptr::null();
libc::setlocale(args.category, null_ptr)
}
Some(l) => {
let mut l_str = l.to_string();
if l_str.is_empty() {
l_str.push('\0');
}
let l_ptr = l_str.as_ptr() as *const i8;
libc::setlocale(args.category, l_ptr)
}
},
};
if result.is_null() {
let error = error(vm);
Expand Down