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

Eric Fiselier via cfe-commits cfe-commits at lists.llvm.org
Mon May 30 22:51:23 PDT 2016


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 <class _Source,
----------------
majnemer wrote:
> 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<int>(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<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)
----------------
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(), &tbuf) == -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.

================
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)  {
----------------
majnemer wrote:
> Formatting?
Not ever sure what I was saying there. Removing it.

================
Comment at: src/experimental/operations.cpp:663
@@ +662,3 @@
+        si.capacity = si.free = si.available =
+            static_cast<decltype(si.free)>(-1);
+        return si;
----------------
majnemer wrote:
> Why not use `uintmax_t` instead of `decltype(si.free)` ?
Not sure what I was thinking. Changing it.

================
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;
----------------
majnemer wrote:
> Does the standard give any guidance if this calculation overflows?
It says:

>  Any members for which the value cannot be determined shall be set to static_cast<uintmax_t>(-1).

I overlooked overflow checking. I'll add it.

No idea how I'll test it though.

================
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;
----------------
majnemer wrote:
> Why all these environment variables?
That's the example implementation given in the spec:
https://rawgit.com/cplusplus/filesystem-ts/develop/working-draft.html#temp_directory_path


http://reviews.llvm.org/D16948





More information about the cfe-commits mailing list