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)) }
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
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. 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