Filesystem i18n mini-review
Been on vacation for a couple of weeks and missed the filesystem mini-review deadline. 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. 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 pair of iterators instead. 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. - What is your evaluation of the implementation no comment - What is your evaluation of the documentation? Very good. - Did you try to use the library? With what compiler? Did you have any problems? Haven't tried the i18n version yet. - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I have used the original filesystem library for approx 1 year and I have followed the i18n development closly. - Are you knowledgeable about the problem domain? yes, but I have only used the windows part (i.e. no portability). - Do you think the library should be accepted as a Boost library? yes.
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
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.
Don't understand what you mean. The String is just the internal storage type in the path, similar to char/wchar for basic_string. basic_string doesn't have an interface using the internal storage except c-strings "const char*". I think the path interface should always have a basic_string interface regardless of the internal storage type. One solution could be to have a second template argument which specifies the interface type. template <class StorageStringT, class InterfaceStringT = basic_string<typename StorageStringT::value_type> > class path { path(const InterfaceStringT& str) { append(str.begin(), str.end()); } InterfaceStringT file_string() const { ... } const StorageStringT& string() const { return m_path; } ... };
Anyhow, the consistent (and sensible) set for path would be:
const path & const string & const char * InputIterator, InputIterator
The last one could be confusing since on a path you iterate over path elements and not characters. string s("/abc/def"); path p1(s.begin(), s.end()); path p2(p1.begin(), p1.end()); // should this work?
participants (2)
-
Beman Dawes -
Martin