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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
