"Martin" <adrianm@touchdown.se> wrote in message news:loom.20050330T110611-38@post.gmane.org...
Didn't find any comments on the new filesystem implementation so here are mine
- Would it be possible to store the new files in the vault? I find it inconvenient to check out a special branch just to read the documentation.
I'll do that after then next round of updates.
- I haven't compiled the new files but I have had a good look at the documentation and the source. My impression is that it is a very nice implementation. (All the #ifdefs make the source hard to follow but I'm not the maintainer)
Thanks. The #ifdef situation has improved since the last release in that there are fewer #ifdefs covering bigger blocks of code, and in some cases overloading has replaced #ifdefs. The only other thing I can think of would be to have separate POSIX and Windows implementation files. But that brings up code duplication issues. Changes to allow the code to work with broken compilers is also likely to introduce more #ifdefs. It's messy, that's for sure.
- Specially I like the switch from default portable syntax to default native syntax. It means that I no longer need to change the default name checker in each application and that the "native_" prefix from the access functions are gone. (Maybe the name checker could be a template parameter (with default "none") for those who still want to use it. Would complicate implementation of operations.)
I thought of that, but decided the symplification from removing the name checking from the basic_path class was more important.
The comments I have on the new implementation is: - Operations are limited to std::string (and std::wstring on win32) as external type. Why not allow any range of characters? Current implementation will not work with const_string, flex_string and a basic_string with a custom allocator.
That isn't correct. See lpath.hpp and wide_test.cpp in the test directory for an example of using a basic_string<long> as the the underlying string type. The docs need more work to make clearer exactly what is required of the String argument to the basic_path template. A programmer wishing to basic_path with something other than std::string or std::wstring has to do more work, but it isn't particularly difficult, as the lpath example shows.
- Why not have a static locale member in the class instead of using the utf-8 facet. The locale could then be used both for internal/external conversion via the codecvt facet and get operator< to work with a custom locale. (for posix the locale would default to locale::global() with the utf-8 facet added)
I'll give that some thought.
I also had hopes for some new things that didn't appear in the new implementation. - non-throwing is_* functions. I find it very inconvenient and error prune to put try/catch around each is_directory call. Calling is_accessible before every other is_* function is an option but not much easier and still leaves a risk for an exception if another process is accessing the file.
If the is_* functions return false, rather than throwing, then testing for the negative of a is_* function becomes unreliable. Does !is_directory( "foo" ) mean that "foo" is a file, or is is just missing? The programmer would have to write exists("foo") && is_accessible("foo") && !is_directory("foo"). That could be simplified by providing an is_file() function (as suggested by Jeff Garland recently), but then we have to define exactly what a file is. Is a socket a file? Is a device a file? Or what POSIX calls a "regular file"? How does that translate to Windows? I'm not saying that approach would be a bad thing. In fact, I was just working out examples of how it might work this morning. But it is a considerable change. Those changes would have to be worked out in detail so we could evaluate them compared to the status quo.
The throwing is_* functions also makes it impossible(?) to use the directory iterator with algorithms like remove_copy_if(directory_iterator(path), directory_iterator(), back_inserter(filelist), is_directory)
While that is an interesting example, note that a user provided function would work instead.
My suggestion is to add non-throwing overloads where a second parameter tells what the function should return in case of error. e.g. is_directory(path, true).
Interesting. That would cover my example about of wanting to detect a file. The expressions would be written !is_directory("foo",true). But that seems a bit obscure to me, and likely to be a cause of coding errors. Hum... it isn't even correct in the case a directory exists but is not accessible An alternative would be named functions. These would be the so called "compound" functions which were discussed at one time, but that gets really messy. Where do you stop?
- The directory iterator is still very limited since you can't specify a filter (e.g. "*.txt"). If used on a win32 system with a mounted posix disk or a posix system with a CD problems might arise. (There is no portable way to tell if "FILE.TXT" matches "*.txt").
There was discussion of a glob directory iterator at one time, but no one ever came up with a final design. It is certainly a real need.
- Why doesn't last_write_time return a boost::ptime
That was a concious decision to limit coupling. That is, to avoid Boost libraries which are not part of TR1. I'd like the Boost version to be identical to what gets proposed to the standards committee, and unless Boost Date-Time gets proposed and accepted, that means it can't be used. Thanks for your comments. Because of the upcoming standards committee meeting, I'll be pretty distracted for the next three weeks, but will try to give some more thought at odd moments to the issues you've raised. --Beman