[PATCH] D16948: [libcxx] Filesystem TS -- Complete

[David Majnemer via cfe-commits]

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 <class _Source,
----------------
TODO?

================
Comment at: src/experimental/directory_iterator.cpp:47
@@ +46,3 @@
+    int ret;
+    if ((ret = ::readdir_r(dir_stream,  &dir_entry,  &dir_entry_ptr)) != 0)
+    {
----------------
`readdir_r` is deprecated, please use `readdir`.

================
Comment at: src/experimental/directory_iterator.cpp:100
@@ +99,3 @@
+                close();
+                //__entry_ = {};
+                return false;
----------------
Why is this commented out?

================
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);
+}
----------------
What sort of logic is this trying to determine?

================
Comment at: src/experimental/operations.cpp:354-355
@@ +353,4 @@
+    if (ec) ec->clear();
+    if (::mkdir(p.c_str(), static_cast<int>(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<seconds>(dur_since_epoch);
+    auto ns_since_epoch = duration_cast<nanoseconds>(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, &m_ec);
+    if (!m_ec) {
+        ::utimbuf tbuf;
+        tbuf.actime = st.st_atime;
+        tbuf.modtime = Clock::to_time_t(new_time);
+        if (::utime(p.c_str(), &tbuf) == -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(), &tbuf) == -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(), &m_svfs) == -1)  {
----------------
Formatting?

================
Comment at: src/experimental/operations.cpp:663
@@ +662,3 @@
+        si.capacity = si.free = si.available =
+            static_cast<decltype(si.free)>(-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