Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF closed this revision. EricWF added a comment. Committed as r273034. http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF accepted this revision. EricWF added a reviewer: EricWF. EricWF added a comment. This revision is now accepted and ready to land. Accepting before committing. Speak now or forever hold your peace. (Or just speak post commit) http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. In http://reviews.llvm.org/D16948#457266, @EricWF wrote: > Add `LIBCXX_ENABLE_FILESYSTEM` CMake option to turn off only the filesystem > parts of `libc++experimental.a`. Thanks! http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. > > Fine by me. > > > > > > I didn't have much time to go through the patch, is there a way to disable > > this module from building? It won't compile for our targets until we put in > > some sort of a porting layer (which I need to start thinking of). Would be > > nice if there is some CMake switch to turn this off until such time. > > > Currently you can switch it off using the CMake option > `-DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF`. This turns off all of the > experimental libraries, including filesystem and polymorphic memory resources. > > Would this be sufficient for your use case? Otherwise I can create an option > to specifically disable filesystem. We can currently build the rest of the experimental library (polymorphic memory resource), so it would be great if we can selectively disable just the filesystem module. Thanks. (sorry for the late response - went to bed) / Asiri http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. In http://reviews.llvm.org/D16948#456966, @rmaprath wrote: > In http://reviews.llvm.org/D16948#456929, @EricWF wrote: > > > Unless there are any objections I plan to commit this change on Friday. > > > > That will give it about a month of in-tree time before the 3.9 release > > branch. > > > Fine by me. > > I didn't have much time to go through the patch, is there a way to disable > this module from building? It won't compile for our targets until we put in > some sort of a porting layer (which I need to start thinking of). Would be > nice if there is some CMake switch to turn this off until such time. Currently you can switch it off using the CMake option `-DLIBCXX_ENABLE_EXPERIMENTAL_LIBRARY=OFF`. This turns off all of the experimental libraries, including filesystem and polymorphic memory resources. Would this be sufficient for your use case? Otherwise I can create an option to specifically disable filesystem. http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. In http://reviews.llvm.org/D16948#456929, @EricWF wrote: > Unless there are any objections I plan to commit this change on Friday. > > That will give it about a month of in-tree time before the 3.9 release branch. Fine by me. I didn't have much time to go through the patch, is there a way to disable this module from building? It won't compile for our targets until we put in some sort of a porting layer (which I need to start thinking of). Would be nice if there is some CMake switch to turn this off until such time. Thanks! / Asiri http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. Unless there are any objections I plan to commit this change on Friday. That will give it about a month of in-tree time before the 3.9 release branch. http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. @mclow.lists Just a reminder that 3.9 branches for release on July 19th. http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF marked an inline comment as done.
Comment at: src/experimental/operations.cpp:128-129
@@ +127,4 @@
+bool stat_equivalent(struct ::stat& st1, struct ::stat& st2) {
+return (st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino);
+}
+
majnemer wrote:
> It is possible for `st_ino` to wrap around while the machine is still running.
> I'd mix `st_gen` into the comparison if we are running under one of the BSDs
> or Darwin.
>
> Linux has a `FS_IOC_GETVERSION` ioctl but it requires a file descriptor.
> Maybe such heroics are not worth it.
> It is possible for st_ino to wrap around while the machine is still running.
Can you clarify this point? Maybe some docs I could read.
> I'd mix st_gen into the comparison if we are running under one of the BSDs or
> Darwin.
According to the darwin docs st_gen is only available as a super user and even
then my tests show it's always zero.
https://developer.apple.com/library/ios/documentation/System/Conceptual/ManPages_iPhoneOS/man2/stat.2.html
How does mixing that in help if st_ino wraps?
Comment at: test/std/utilities/meta/meta.rel/is_nothrow_callable.pass.cpp:57
@@ -56,3 +56,3 @@
struct Dummy { void foo() noexcept {} static void bar() noexcept {} };
-#if !defined(__cpp_noexcept_function_type)
+#if !defined(__cpp_noexcept_function_type) && 0
{
ahatanak wrote:
> Can you explain why this change was needed? I'm trying to figure out why the
> two static_asserts in the #else part fail.
Woops that sneaked in. I was investigating some GCC behavior unrelated to
filesystem but forgot to revert the changes.
http://reviews.llvm.org/D16948
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
ahatanak added a subscriber: ahatanak.
Comment at: test/std/utilities/meta/meta.rel/is_nothrow_callable.pass.cpp:57
@@ -56,3 +56,3 @@
struct Dummy { void foo() noexcept {} static void bar() noexcept {} };
-#if !defined(__cpp_noexcept_function_type)
+#if !defined(__cpp_noexcept_function_type) && 0
{
Can you explain why this change was needed? I'm trying to figure out why the
two static_asserts in the #else part fail.
http://reviews.llvm.org/D16948
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
majnemer added inline comments.
Comment at: src/experimental/operations.cpp:128-129
@@ +127,4 @@
+bool stat_equivalent(struct ::stat& st1, struct ::stat& st2) {
+return (st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino);
+}
+
It is possible for `st_ino` to wrap around while the machine is still running.
I'd mix `st_gen` into the comparison if we are running under one of the BSDs or
Darwin.
Linux has a `FS_IOC_GETVERSION` ioctl but it requires a file descriptor. Maybe
such heroics are not worth it.
http://reviews.llvm.org/D16948
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
majnemer added inline comments.
Comment at: src/experimental/operations.cpp:529
@@ +528,3 @@
+
+if (::utimensat(AT_FDCWD, p.c_str(), tbuf, 0) == -1) {
+m_ec = detail::capture_errno();
SUSv4 says:
> The utime() function is marked obsolescent.
However, `utimes` was made legacy in SUSv3 and removed from legacy in SUSv4
http://pubs.opengroup.org/onlinepubs/9699919799/functions/utimes.html
http://reviews.llvm.org/D16948
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. In http://reviews.llvm.org/D16948#444013, @EricWF wrote: > The git branch I develop on is public: > https://github.com/efcs/libcxx/tree/filesystem-ts > > You can download or clone from there (and it will always be up to date!). All tests pass! :-) I'll try to find an arm-linux setup soon and run this there as well. / Asiri http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. The git branch I develop on is public: https://github.com/efcs/libcxx/tree/filesystem-ts You can download or clone from there (and it will always be up to date!). http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. In http://reviews.llvm.org/D16948#444009, @rmaprath wrote: > Would you / community be open to the idea of hiding the os syscalls behind an > API? (like we are doing for pthreads)? Yes I would be very open to that. Then I could also have test shims in order to test truly exceptional cases. > I think this is the only way we could get at least some of this functionality > working on bare-metal ARM (it should work on arm-linux without much trouble - > I think). How much work do you think this refactoring would need? I'm happy > to do that after you land the patch. It shouldn't be that much work to get `path`, `directory_iterator` and `recursive_directory_iterator` working. They have like 4 system calls in there implementation. The rest of the TS is just a set of free functions. Each uses a separate system call for the most part. However the API is very simple and you can pick and choose which functions you want to support. > I will see if I can try this out on arm-linux, as soon as I resolve this > fedora20 mystery of mine :) > > Cheers, > > / Asiri http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. In http://reviews.llvm.org/D16948#444010, @EricWF wrote: > Ah I figured it out! The test suite has symlinks within it. Git handles them > properly but they obviously don't survive the round trip to phabricator and > back. So all of the tests that depend on them fail (with is evidently a lot). Perhaps you can put up a tarball somewhere? / Asiri http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. Ah I figured it out! The test suite has symlinks within it. Git handles them properly but they obviously don't survive the round trip to phabricator and back. So all of the tests that depend on them fail (with is evidently a lot). http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. Would you / community be open to the idea of hiding the os syscalls behind an API? (like we are doing for pthreads)? I think this is the only way we could get at least some of this functionality working on bare-metal ARM (it should work on arm-linux without much trouble - I think). How much work do you think this refactoring would need? I'm happy to do that after you land the patch. I will see if I can try this out on arm-linux, as soon as I resolve this fedora20 mystery of mine :) Cheers, / Asiri http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. In http://reviews.llvm.org/D16948#444006, @rmaprath wrote: > In http://reviews.llvm.org/D16948#443999, @EricWF wrote: > > > I installed a Fedora 23 system and ran the test suite and I didn't see any > > of the same failures. It must be something other than the distro. > > Are you using an odd filesystem? Or maybe it could be your system locale? > > > I suspect so. Ran the latest patch just now and I'm still seeing the > remaining failures. > > Btw, when do you plan to land this? I'm a bit tight as we are on the run-up > to a release. I will investigate my failures offline, but I don't think it > should be blocking you (given that you've got them working on fedora 23). > Mind you fedora 20 is a bit old. > > / Asiri I don't have an odd filesystem btw, I've let fedora manage the partitioning etc. (LVM stuff - I don't know much). Other than that, I have an SSD (but don't think it can affect these tests). / Asiri http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. In http://reviews.llvm.org/D16948#444006, @rmaprath wrote: > In http://reviews.llvm.org/D16948#443999, @EricWF wrote: > > > I installed a Fedora 23 system and ran the test suite and I didn't see any > > of the same failures. It must be something other than the distro. > > Are you using an odd filesystem? Or maybe it could be your system locale? > > > I suspect so. Ran the latest patch just now and I'm still seeing the > remaining failures. > > Btw, when do you plan to land this? I'm a bit tight as we are on the run-up > to a release. I will investigate my failures offline, but I don't think it > should be blocking you (given that you've got them working on fedora 23). > Mind you fedora 20 is a bit old. > > / Asiri I have no immediate plans to land it. I'm going to try and get some eyeballs on it and some other people to try it first. If it lands and starts causing large amounts of failures it's going to be a headache and likely get reverted. http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. In http://reviews.llvm.org/D16948#443999, @EricWF wrote: > I installed a Fedora 23 system and ran the test suite and I didn't see any of > the same failures. It must be something other than the distro. > Are you using an odd filesystem? Or maybe it could be your system locale? I suspect so. Ran the latest patch just now and I'm still seeing the remaining failures. Btw, when do you plan to land this? I'm a bit tight as we are on the run-up to a release. I will investigate my failures offline, but I don't think it should be blocking you (given that you've got them working on fedora 23). Mind you fedora 20 is a bit old. / Asiri http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. In http://reviews.llvm.org/D16948#443945, @rmaprath wrote: > In http://reviews.llvm.org/D16948#443942, @EricWF wrote: > > > In http://reviews.llvm.org/D16948#443941, @rmaprath wrote: > > > > > I'm seeing a few failures on my Fedora 20 system. Will go through these > > > tomorrow. > > > > > > If you want to just send me the raw ouput I'll go over them tonight. > > > This is from a default (without extra cmake options) build: > https://dl.dropboxusercontent.com/u/12212624/Other/filesystem_libcxx_results_fedora20.log > > Some of them look obvious. For the rest, let me know if I can get you more > info, I can debug tomorrow evening. I installed a Fedora 23 system and ran the test suite and I didn't see any of the same failures. It must be something other than the distro. Are you using an odd filesystem? Or maybe it could be your system locale? http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF marked 6 inline comments as done.
EricWF added a comment.
@majnemer Thanks for looking at this!
Comment at: include/experimental/filesystem:610
@@ +609,3 @@
+static void __append_range(string& __dest, _Iter __b, _Iter __e) {
+// TODO(EricWF) We get better allocation behavior here if we don't
+// provide the same exception safety guarantees as string.append.
majnemer wrote:
> Do we typically put names in TODOs?
IDK. I normally do?
Comment at: include/experimental/filesystem:672
@@ +671,3 @@
+
+// TODO
+template TODO?
I'll clarify that. I have no idea how to implement the locale conversions. I
was going to wait until most of filesystem landed before worrying about them.
Comment at: src/experimental/directory_iterator.cpp:100
@@ +99,3 @@
+close();
+//__entry_ = {};
+return false;
majnemer wrote:
> Why is this commented out?
It should be removed. After the stream is closed the __entry_ member is never
referenced again.
Comment at: src/experimental/operations.cpp:127-128
@@ +126,4 @@
+
+bool stat_equivalent(struct ::stat& st1, struct ::stat& st2) {
+return (st1.st_dev == st2.st_dev && st1.st_ino == st2.st_ino);
+}
majnemer wrote:
> What sort of logic is this trying to determine?
If two paths resolve the the same file according to
https://rawgit.com/cplusplus/filesystem-ts/develop/working-draft.html#equivalent.
Comment at: src/experimental/operations.cpp:354-355
@@ +353,4 @@
+if (ec) ec->clear();
+if (::mkdir(p.c_str(), static_cast(perms::all)) == 0)
+return true;
+if (errno != EEXIST || !is_directory(p))
majnemer wrote:
> Any reason why `mkdir` isn't wrapped like the other fs calls?
There are plenty of other fs calls that aren't wrapped. The wrapped ones are
because they are used in a bunch of places.
Comment at: src/experimental/operations.cpp:381
@@ +380,3 @@
+std::error_code *ec){
+if (::symlink(from.c_str(), to.c_str()) != 0)
+set_or_throw(ec, "create_directory_symlink", from, to);
majnemer wrote:
> Hmm, why compare against zero instead of -1?
Probably an oversight.
Comment at: src/experimental/operations.cpp:403
@@ +402,3 @@
+path __current_path(std::error_code *ec) {
+auto size = ::pathconf(".", _PC_PATH_MAX);
+_LIBCPP_ASSERT(size >= 0, "pathconf returned a 0 as max size");
majnemer wrote:
> Hmm, SUSv4 says:
> > The value returned for the variable {PATH_MAX} indicates the longest
> > relative pathname that could be given if the specified directory is the
> > current working directory of the process.
>
> However, you want an absolute path relative to the root directory right?
Yes, but that language confuses me. If I'm not mistaken the longest relative
pathname for the CWD is going to be longer than the absolute one, since it's
easy to take an absolute path and add "./././././././" and still refer to the
same directory.
FYI this code also appears in tho opergroup spec for `getcwd`.
http://pubs.opengroup.org/onlinepubs/009695399/functions/getcwd.html
Comment at: src/experimental/operations.cpp:506-532
@@ +505,29 @@
+std::error_code m_ec;
+#if !defined(__APPLE__)
+using namespace std::chrono;
+auto dur_since_epoch = new_time.time_since_epoch();
+auto sec_since_epoch = duration_cast(dur_since_epoch);
+auto ns_since_epoch = duration_cast(dur_since_epoch -
sec_since_epoch);
+struct ::timespec tbuf[2];
+tbuf[0].tv_sec = 0;
+tbuf[0].tv_nsec = UTIME_OMIT;
+tbuf[1].tv_sec = sec_since_epoch.count();
+tbuf[1].tv_nsec = ns_since_epoch.count();
+
+if (::utimensat(AT_FDCWD, p.c_str(), tbuf, 0) == -1) {
+m_ec = detail::capture_errno();
+}
+#else
+using Clock = file_time_type::clock;
+struct ::stat st;
+detail::posix_stat(p, st, _ec);
+if (!m_ec) {
+::utimbuf tbuf;
+tbuf.actime = st.st_atime;
+tbuf.modtime = Clock::to_time_t(new_time);
+if (::utime(p.c_str(), ) == -1) {
+m_ec = detail::capture_errno();
+}
+}
+#endif
+if (m_ec)
majnemer wrote:
> I'd suggest you swap these two blocks around. This way we don't need to add
> more conjunctions if we add more special cases.
Ack.
Comment at: src/experimental/operations.cpp:528
@@ +527,3 @@
+tbuf.modtime = Clock::to_time_t(new_time);
+if (::utime(p.c_str(), ) == -1) {
+m_ec = detail::capture_errno();
majnemer wrote:
> Isn't `utime` deprecated? I'd suggest using `utimes`.
If I'm not mistaken `utimes` is legacy, not `utime`. Also note that `utimensat`
is used on all platforms except for apple, for which this is the fallback
implementation.
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
majnemer added a subscriber: majnemer.
Comment at: include/experimental/filesystem:610
@@ +609,3 @@
+static void __append_range(string& __dest, _Iter __b, _Iter __e) {
+// TODO(EricWF) We get better allocation behavior here if we don't
+// provide the same exception safety guarantees as string.append.
Do we typically put names in TODOs?
Comment at: include/experimental/filesystem:672
@@ +671,3 @@
+
+// TODO
+template clear();
+if (::mkdir(p.c_str(), static_cast(perms::all)) == 0)
+return true;
+if (errno != EEXIST || !is_directory(p))
Any reason why `mkdir` isn't wrapped like the other fs calls?
Comment at: src/experimental/operations.cpp:381
@@ +380,3 @@
+std::error_code *ec){
+if (::symlink(from.c_str(), to.c_str()) != 0)
+set_or_throw(ec, "create_directory_symlink", from, to);
Hmm, why compare against zero instead of -1?
Comment at: src/experimental/operations.cpp:403
@@ +402,3 @@
+path __current_path(std::error_code *ec) {
+auto size = ::pathconf(".", _PC_PATH_MAX);
+_LIBCPP_ASSERT(size >= 0, "pathconf returned a 0 as max size");
Hmm, SUSv4 says:
> The value returned for the variable {PATH_MAX} indicates the longest relative
> pathname that could be given if the specified directory is the current
> working directory of the process.
However, you want an absolute path relative to the root directory right?
Comment at: src/experimental/operations.cpp:506-532
@@ +505,29 @@
+std::error_code m_ec;
+#if !defined(__APPLE__)
+using namespace std::chrono;
+auto dur_since_epoch = new_time.time_since_epoch();
+auto sec_since_epoch = duration_cast(dur_since_epoch);
+auto ns_since_epoch = duration_cast(dur_since_epoch -
sec_since_epoch);
+struct ::timespec tbuf[2];
+tbuf[0].tv_sec = 0;
+tbuf[0].tv_nsec = UTIME_OMIT;
+tbuf[1].tv_sec = sec_since_epoch.count();
+tbuf[1].tv_nsec = ns_since_epoch.count();
+
+if (::utimensat(AT_FDCWD, p.c_str(), tbuf, 0) == -1) {
+m_ec = detail::capture_errno();
+}
+#else
+using Clock = file_time_type::clock;
+struct ::stat st;
+detail::posix_stat(p, st, _ec);
+if (!m_ec) {
+::utimbuf tbuf;
+tbuf.actime = st.st_atime;
+tbuf.modtime = Clock::to_time_t(new_time);
+if (::utime(p.c_str(), ) == -1) {
+m_ec = detail::capture_errno();
+}
+}
+#endif
+if (m_ec)
I'd suggest you swap these two blocks around. This way we don't need to add
more conjunctions if we add more special cases.
Comment at: src/experimental/operations.cpp:528
@@ +527,3 @@
+tbuf.modtime = Clock::to_time_t(new_time);
+if (::utime(p.c_str(), ) == -1) {
+m_ec = detail::capture_errno();
Isn't `utime` deprecated? I'd suggest using `utimes`.
Comment at: src/experimental/operations.cpp:659
@@ +658,3 @@
+struct statvfs m_svfs;
+//if we fail but don't throw
+if (::statvfs(p.c_str(), _svfs) == -1) {
Formatting?
Comment at: src/experimental/operations.cpp:663
@@ +662,3 @@
+si.capacity = si.free = si.available =
+static_cast(-1);
+return si;
Why not use `uintmax_t` instead of `decltype(si.free)` ?
Comment at: src/experimental/operations.cpp:667-669
@@ +666,5 @@
+if (ec) ec->clear();
+si.capacity = m_svfs.f_blocks * m_svfs.f_frsize;
+si.free = m_svfs.f_bfree * m_svfs.f_frsize;
+si.available = m_svfs.f_bavail * m_svfs.f_frsize;
+return si;
Does the standard give any guidance if this calculation overflows?
Comment at: src/experimental/operations.cpp:687
@@ +686,3 @@
+path __temp_directory_path(std::error_code *ec) {
+const char* env_paths[] = {"TMPDIR", "TMP", "TEMP", "TEMPDIR"};
+const char* ret = nullptr;
Why all these environment variables?
Comment at: src/experimental/operations.cpp:705-707
@@ +704,5 @@
+
+//since the specification of absolute in the current specification
+// this implementation is designed after the sample implementation
+// using the sample table as a guide
+path absolute(const path& p, const path& base) {
Formatting?
http://reviews.llvm.org/D16948
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. Thanks. I fixed the missing include causing most of the failures. Please update me when you can with new results. http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. In http://reviews.llvm.org/D16948#443942, @EricWF wrote: > In http://reviews.llvm.org/D16948#443941, @rmaprath wrote: > > > I'm seeing a few failures on my Fedora 20 system. Will go through these > > tomorrow. > > > If you want to just send me the raw ouput I'll go over them tonight. This is from a default (without extra cmake options) build: https://dl.dropboxusercontent.com/u/12212624/Other/filesystem_libcxx_results_fedora20.log Some of them look obvious. For the rest, let me know if I can get you more info, I can debug tomorrow evening. / Asiri http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. In http://reviews.llvm.org/D16948#443941, @rmaprath wrote: > I'm seeing a few failures on my Fedora 20 system. Will go through these > tomorrow. If you want to just send me the raw ouput I'll go over them tonight. http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a comment. I'm seeing a few failures on my Fedora 20 system. Will go through these tomorrow. http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. In http://reviews.llvm.org/D16948#438880, @rmaprath wrote: > Looks like I've completely missed this patch somehow. Will try to find some > time (or someone) to have a look at it from an embedded-systems / ARM point > of view asap. > > Great work!!! Thanks! Even just attempting to run the tests is greatly appreciated. Writing portable tests is next to impossible so there are going to be kinks to iron out for each platform. http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
rmaprath added a subscriber: rmaprath. rmaprath added a comment. Looks like I've completely missed this patch somehow. Will try to find some time (or someone) to have a look at it from an embedded-systems / ARM point of view asap. Great work!!! http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D16948: [libcxx] Filesystem TS -- Complete
EricWF added a comment. For reference here's the current test coverage: http://efcs.ca/filesystem-coverage/ http://reviews.llvm.org/D16948 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
