You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Mmh, in Modules/python.c and Python/frozenmain.c, the return value of
the second call to mbstowcs() should be checked as well (since the first
one is sometimes replaced by a call to strlen(), which never fails).
henryprecheur commented on Aug 28, 2008
henryprecheurmannequin
MannequinAuthor
More actions
I removed my previous patch. It was not dealing with all broken mbstowcs
cases and yours is much better.
Some comments on the other patch:
I don't think the macro HAVE_BROKEN_MBSTOWCS alone is a perfect idea. It
won't fix the problem in the long run:
Most contributors won't be aware of this problem and might be using
mbstowcs without putting the #ifdef's. Then the problem will come back
and bite us again.
Simply because it easier to get "Be positive" than "Don't be negative".
The negative form is generally harder to get whether your are
programming, writing or talking. Use the positive form and you will have
more impact and will make things clearer.
Another small remark; #ifdef is better then #ifndef
Simply because it easier to get "Be positive" than "Don't be negative".
Yes; but here, the symbol (HAVE_BROKEN_MBSTOWC) has a negative meaning.
I tried to put the optimistic (=not broken) case first.
However, if this makes the code more difficult to read, I'll change it.
Yes; but here, the symbol (HAVE_BROKEN_MBSTOWC) has a negative meaning.
I tried to put the optimistic (=not broken) case first.
However, if this makes the code more difficult to read, I'll change it.
You could change HAVE_BROKEN_MBSTOWC for a positive flag, e.g.
HAVE_WORKING_MBSTOWC, and then the #ifdef would be the optimistic case.
Amaury, as long as you fix the small quirk mentioned above (checking the
return value of the second call to mbstowcs()), I think this patch can
go in, since it does no harm on already working platforms.
Activity
henryprecheur commentedon Aug 27, 2008
The function mbstowcs is buggy on OpenBSD <= 4.4:
http://www.openbsd.org/cgi-bin/cvsweb/src/lib/libc/locale/multibyte_sb.c#rev1.7
Because of this the following command fails:
./python -E setup.py build
The attached patch fixes the problem.
[-]Error parsing arguments on OpenBSD <= 4.4[/-][+]mbstowcs[/+][-]mbstowcs[/-][+]Error parsing arguments on OpenBSD <= 4.4[/+]amauryfa commentedon Aug 27, 2008
See also bpo-3626, which is exactly the same for cygwin.
Maybe a "./configure" paragraph could discover whether mbstowcs is broken.
I don't know how to do it though.
amauryfa commentedon Aug 27, 2008
Here is another patch, which tries to replace all problematic usages of
mbstowcs, and a new "#define HAVE_BROKEN_MBSTOWCS" to activate it.
It needs a review, especially the "configure" stuff, it's my first time
to deal with this.
Tested on cygwin.
pitrou commentedon Aug 27, 2008
The patch looks fine and harmless to me (but I'm a configure newbie
too). +1 for committing it and seeing if the OpenBSD buildbot feels better.
In the process of testing this patch, I've found another bug: bpo-3705.
pitrou commentedon Aug 27, 2008
Mmh, in Modules/python.c and Python/frozenmain.c, the return value of
the second call to mbstowcs() should be checked as well (since the first
one is sometimes replaced by a call to strlen(), which never fails).
henryprecheur commentedon Aug 28, 2008
I removed my previous patch. It was not dealing with all broken mbstowcs
cases and yours is much better.
Some comments on the other patch:
I don't think the macro HAVE_BROKEN_MBSTOWCS alone is a perfect idea. It
won't fix the problem in the long run:
Most contributors won't be aware of this problem and might be using
mbstowcs without putting the #ifdef's. Then the problem will come back
and bite us again.
I would rather do something like this:
It would fix the problem everywhere, and people won't have to worry
about putting #ifdef everywhere in the future.
I attached a test program, run it on cygwin to make sure this approach
works on your side.
Simply because it easier to get "Be positive" than "Don't be negative".
The negative form is generally harder to get whether your are
programming, writing or talking. Use the positive form and you will have
more impact and will make things clearer.
amauryfa commentedon Aug 28, 2008
Yes; but here, the symbol (HAVE_BROKEN_MBSTOWC) has a negative meaning.
I tried to put the optimistic (=not broken) case first.
However, if this makes the code more difficult to read, I'll change it.
pitrou commentedon Aug 28, 2008
You could change HAVE_BROKEN_MBSTOWC for a positive flag, e.g.
HAVE_WORKING_MBSTOWC, and then the #ifdef would be the optimistic case.
pitrou commentedon Sep 3, 2008
Amaury, as long as you fix the small quirk mentioned above (checking the
return value of the second call to mbstowcs()), I think this patch can
go in, since it does no harm on already working platforms.
amauryfa commentedon Sep 3, 2008
Here is an updated patch:
exact number of chars.
I won't have SVN access for the next couple of days (behind a firewall...)
Antoine (or someone else), can you please check this in?
pitrou commentedon Sep 3, 2008
Committed in r66187. Henry, could you confirm it fixes the problem on
your side?
henryprecheur commentedon Sep 4, 2008
I am now able to finish the build and the interpreter seems to be
working. So it is all good :)