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

Wed Apr 6 09:28:33 PDT 2022

ldionne accepted this revision.
ldionne added a comment.

LGTM, I think all comments have been addressed but please confirm with Costa before shipping, just to be sure.

================
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:
> var-const wrote:
> > 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.
> I think a TODO doesn't make a lot of sense here. This commented out line exists in almost every single file in `strings/basic.string`. It just makes it harder to remove all the residue mechanically.
I agree with Costa in the general case, however in this specific instance the `string` tests have this commented out bit consistently, so I think this is fine as-is. Also we know we're going to uncomment it in the very short term.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D123129