[libcxx-commits] [PATCH] D123129: [libc++] Add tests for std::string default constructor and destructor

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/default.pass.cpp:13
+
+// This tests a conforming extension
+
----------------
This comment is no longer entirely true, because now this file also tests that the default constructor works and creates an empty string. I'd suggest moving it closer to the static assertions and rephrasing to something like `Test the noexcept specification, which is a conforming extension.`.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/default.pass.cpp:13
+
+// This tests a conforming extension
+
----------------
var-const wrote:
> This comment is no longer entirely true, because now this file also tests that the default constructor works and creates an empty string. I'd suggest moving it closer to the static assertions and rephrasing to something like `Test the noexcept specification, which is a conforming extension.`.
If this is an extension, it seems like the test should use `LIBCPP_STATIC_ASSERT` (unless the other implementations just happen to do the same extension). If this test passes with GCC and MSVC, I wonder if the extension in question is fully tested?


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/default.pass.cpp:40
+#if TEST_STD_VER > 17
+  // static_assert(test());
+#endif
----------------
philnik wrote:
> nilayvaish wrote:
> > Why is this commented out? Same question for the other file as well.
> `std::string` isn't constexpr yet. This will be uncommented when `std::string` is made constexpr (D110598 if you want to review it :P).
Can you please add a TODO to uncomment this line once that patch lands? In general, I think we should always add a TODO or at least a comment when committing commented out code.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/dtor.pass.cpp:40
+                std::basic_string<char, std::char_traits<char>, test_allocator<char>>>::value, "");
 #if defined(_LIBCPP_VERSION)
+static_assert(!std::is_nothrow_destructible<
----------------
Nit: can you use `LIBCPP_STATIC_ASSERT`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123129