[libcxx-commits] [PATCH] D137640: [libcxx] Implement P2467R1: Support exclusive mode for fstreams

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

Thanks for working on this! Please goo through https://libcxx.llvm.org/Contributing.html#pre-commit-check-list and make sure that you did everything listed there.



================
Comment at: libcxx/include/fstream:557
     return "a+b" _LIBCPP_FOPEN_CLOEXEC_MODE;
+#if defined(__cpp_lib_ios_noreplace)
+  case ios_base::out | ios_base::noreplace:
----------------
We don't want anybody to think that this is configurable. Same for the other places.


================
Comment at: libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
It seems like you are missing tests for everything but `in | out | trunc | noreplace`. Please add them.


================
Comment at: libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp:17
+
+// plate <class charT, class traits = char_traits<charT> >
+// class basic_fstream
----------------



================
Comment at: libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp:29
+
+namespace fs = std::filesystem;
+
----------------
We normally avoid this to make it simpler for readers to see where symbols are coming from. In this case you are using `fs` in a bunch of different places too, making it even harder to read. It also looks like there is just a single use of `fs::` anywhere, so it doesn't even remove any verbosity.


================
Comment at: libcxx/test/std/input.output/file.streams/fstreams/fstream.cons/noreplace.pass.cpp:54
+  }
+  std::remove(p.string().c_str());
+
----------------
Why aren't you using `std::filesystem::remove(p)`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137640/new/

https://reviews.llvm.org/D137640