Re: [boost] Filesystem i18n mini-review

From: Beman Dawes (bdawes_at_[hidden])
Date: 2005-07-12 08:50:42

At 04:10 PM 7/11/2005, Martin wrote:
 >Been on vacation for a couple of weeks and missed the filesystem
 >mini-review deadline.

No problem.

 >Here are som quick comments anyway:
 >
 >- What is your evaluation of the design?
 >
 >I think that the general design is very nice but I have a few issues:
 >
 >1. The basic_path template parameter "String" defines the type used to 
store
 >
 >the path internally but at the same time it
 >also defines the type used for construction and operator/.
 >
 >Consider the following example:
 >I want to store the path in shared memory and therefore use a flex_string 

 >type with a 200 character internal buffer.
 >Problem here is that now the path can only be created from this 200
 >character flex_string and not from std::string.

Ah! Amazing no one (including me) has brought this issue up before, since 
it applies to the current version as well as the i18n version.

Thanks for bring this up.

 >One solution might be to let the constructor and operator/ work with 
ranges
 >but it might create problems with copy constructor.
 >
 >template <typename RangeT>
 >explicit basic_path(const RangeT& str) {
 >  append(begin(str), end(str))
 >}

Since boost::filesystem::basic_path is trying to stay in sync with 
std::basic_string, I'd like to at least consider all the overloads for 
operations provided by std::basic_string.

Hum... Looking at std::basic_string, it is inconsistent. Why am I 
surprised?

Anyhow, the consistent (and sensible) set for path would be:

     const path &
     const string &
     const char *
     InputIterator, InputIterator

I'll add a do-list item to apply those uniformly to all applicable 
constructors and operations.

The two iterator form was used rather than RangeT because I'm trying to 
hold dependencies down. It is very useful for Boost.Filesystem to work well 
even with broken compilers that have trouble supporting Boost.

 >2. All operations work with std::(w)string.
 >
 >When the internal string type is not std::string a conversion is 
necessary.
 >In most cases the charcater conversion should be trivial but since a
 >std::string is required a copy can't be avoided.
 >
 >In the example I used above, the flex_string must be copied into a
 >std::string for each operation even if the character string is identical.
 >
 >Why not base operations on null terminated strings (const char*) or a 
pair
 >of iterators instead.

Some operations already do this for value_type pointers, at least for some 
operations. The fix for (1) above will be applied to all operations.

 >3. Comparison between paths doesn't use locale.
 >
 >The paths are not treated as plain characters since a lexicographical
 >compare is made. But the individual elements are treated
 >as plain characters and the internal string type's operator< is used. 
This
 >is a bit contradicting.
 >
 >I think the comparison operators should compare the elements in a locale
 >dependent way.

I've opened this as an issue. It needs to be answered in the context of 
what comparison is used for. Remember that equivalence should be determined 
by the equivalent() function. An important use of operator< is when path is 
used as a map or set key. Not sure how locale impacts that.

 >
 >...
 >
 >- Do you think the library should be accepted as a Boost library?
 >
 >yes.

Thanks! And thanks for your comments,

--Beman