[1.47.0] Permission to merge filesystem fix
This changeset: http://svn.boost.org/trac/boost/changeset/72855 Has now cycled through many of the regression testers. Testing is still a little light on Windows, but I've tested locally on VC++ 8, 9, and 10, and late model mingw, too. This fixes a nasty POSIX case that was throwing before main() started if the environment LANG variable was whacked. OK to merge? --Beman
On 7/3/2011 2:48 PM, Beman Dawes wrote:
This changeset:
http://svn.boost.org/trac/boost/changeset/72855
Has now cycled through many of the regression testers. Testing is still a little light on Windows, but I've tested locally on VC++ 8, 9, and 10, and late model mingw, too.
This fixes a nasty POSIX case that was throwing before main() started if the environment LANG variable was whacked.
OK to merge?
The changes in the .cpp just look like comments and formatting. Is that right? And the extra path constructors ... are there cases where the new behavior is different than the old? -- Eric Niebler BoostPro Computing http://www.boostpro.com
On Sun, Jul 3, 2011 at 9:24 PM, Eric Niebler <eric@boostpro.com> wrote:
On 7/3/2011 2:48 PM, Beman Dawes wrote:
This changeset:
http://svn.boost.org/trac/boost/changeset/72855
Has now cycled through many of the regression testers. Testing is still a little light on Windows, but I've tested locally on VC++ 8, 9, and 10, and late model mingw, too.
This fixes a nasty POSIX case that was throwing before main() started if the environment LANG variable was whacked.
OK to merge?
The changes in the .cpp just look like comments and formatting. Is that right? And the extra path constructors ... are there cases where the new behavior is different than the old?
The two added constructors are the critical change. Since their argument is already of the required type and encoding, there is no need to access the codecvt facet. And since there is no need for the facet, there is also no need for constructing a std::locale("") object. The path implementation, in src/path.cpp, constructs two paths with the native value_type for internal use. Without the change, GCC thinks that the construction of these two paths can be done as static initialization, before main() runs. Thus if LANG is whacked, the resulting exception from trying to construct std::locale("") is thrown before mainj() starts. By adding the two constructors, that is avoided. It is also a small, but nice, optimization for the most common use case in code that runs on POSIX systems of narrow character paths. Not to mention avoiding the exception entirely if the program does no trafficking in wide character paths. If your eye are glazing over, don't worry. It took me three days to figure it out. Mostly learning to use Code::Blocks on Linux, and then an hour of debugging and testing to see exactly what was happening. --Beman
Apologies for the delay. The US holiday distracted me. On 7/3/2011 7:26 PM, Beman Dawes wrote:
On Sun, Jul 3, 2011 at 9:24 PM, Eric Niebler <eric@boostpro.com> wrote:
On 7/3/2011 2:48 PM, Beman Dawes wrote:
This changeset:
http://svn.boost.org/trac/boost/changeset/72855
Has now cycled through many of the regression testers. Testing is still a little light on Windows, but I've tested locally on VC++ 8, 9, and 10, and late model mingw, too.
This fixes a nasty POSIX case that was throwing before main() started if the environment LANG variable was whacked.
OK to merge?
The changes in the .cpp just look like comments and formatting. Is that right? And the extra path constructors ... are there cases where the new behavior is different than the old?
The two added constructors are the critical change. Since their argument is already of the required type and encoding, there is no need to access the codecvt facet. And since there is no need for the facet, there is also no need for constructing a std::locale("") object.
The path implementation, in src/path.cpp, constructs two paths with the native value_type for internal use. Without the change, GCC thinks that the construction of these two paths can be done as static initialization, before main() runs. Thus if LANG is whacked, the resulting exception from trying to construct std::locale("") is thrown before mainj() starts. By adding the two constructors, that is avoided. It is also a small, but nice, optimization for the most common use case in code that runs on POSIX systems of narrow character paths. Not to mention avoiding the exception entirely if the program does no trafficking in wide character paths.
If your eye are glazing over, don't worry. It took me three days to figure it out. Mostly learning to use Code::Blocks on Linux, and then an hour of debugging and testing to see exactly what was happening.
I'm sure it's fine if you believe there is no behavior change in any supported use case. Let me clarify why I was asking though, because I don't think I was clear. The constructor arguments may already have the required type, but you can't really know anything about their encoding because that's not encoded in the type system. Previously, you were always running these strings through a transformation. You're presuming (and you would know best of course) that the transformation was always a no-op for arguments of this type. I was asking if you were really 100% sure. -- Eric Niebler BoostPro Computing http://www.boostpro.com
On Tue, Jul 5, 2011 at 11:31 AM, Eric Niebler <eric@boostpro.com> wrote:
Let me clarify why I was asking though, because I don't think I was clear. The constructor arguments may already have the required type, but you can't really know anything about their encoding because that's not encoded in the type system. Previously, you were always running these strings through a transformation. You're presuming (and you would know best of course) that the transformation was always a no-op for arguments of this type. I was asking if you were really 100% sure.
Actually, the codecvt argument has always been ignored if the value_types were the same. See the convert() overloads in path_traits.hpp, starting at line 125. The only difference is that by introducing the two constructor value_type overloads there is no call to codecvt(), so no attempt to access a locale that may be invalid. Since the two internal path constructions always have value_type arguments, the changet eliminates the chance of that exception being thrown before main() starts. --Beman
On 7/5/2011 9:32 AM, Beman Dawes wrote:
On Tue, Jul 5, 2011 at 11:31 AM, Eric Niebler <eric@boostpro.com> wrote:
Let me clarify why I was asking though, because I don't think I was clear. The constructor arguments may already have the required type, but you can't really know anything about their encoding because that's not encoded in the type system. Previously, you were always running these strings through a transformation. You're presuming (and you would know best of course) that the transformation was always a no-op for arguments of this type. I was asking if you were really 100% sure.
Actually, the codecvt argument has always been ignored if the value_types were the same. See the convert() overloads in path_traits.hpp, starting at line 125. The only difference is that by introducing the two constructor value_type overloads there is no call to codecvt(), so no attempt to access a locale that may be invalid.
Since the two internal path constructions always have value_type arguments, the changet eliminates the chance of that exception being thrown before main() starts.
Thanks for the clarification. -- Eric Niebler BoostPro Computing http://www.boostpro.com
On 7/6/2011 12:26 AM, Eric Niebler wrote:
Thanks for the clarification.
Can you clarify? ;-) Does that mean it's OK for Beman to do the merge? -- -- Grafik - Don't Assume Anything -- Redshift Software, Inc. - http://redshift-software.com -- rrivera/acm.org (msn) - grafik/redshift-software.com -- 102708583/icq - grafikrobot/aim,yahoo,skype,efnet,gmail
On 7/6/2011 9:31 AM, Rene Rivera wrote:
On 7/6/2011 12:26 AM, Eric Niebler wrote:
Thanks for the clarification.
Can you clarify? ;-) Does that mean it's OK for Beman to do the merge?
Oh, I'm sorry, I thought I covered that when I said, "I'm sure it's fine if you believe there is no behavior change in any supported use case." YES, please go ahead. -- Eric Niebler BoostPro Computing http://www.boostpro.com
participants (3)
-
Beman Dawes -
Eric Niebler -
Rene Rivera