[libcxx-commits] [PATCH] D106801: [libc++] [c++2b] [P2166] Prohibit string and string_view construction from nullptr.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added inline comments.


================
Comment at: libcxx/test/std/strings/basic.string/string.cons/nullptr.fail.cpp:18
+int main(int, char**) {
+  std::string s(nullptr); // expected-error {{call to deleted constructor}}
+
----------------
(1) I think we should also test at least one of the implicit-conversion syntaxes: `std::string s = nullptr;` and/or `void f(std::string); f(nullptr);`
(2) Personally I see no problem with testing explicit construction, implicit conversion, and assignment all in the same test file (but defer to @ldionne if he has an opinion).
(3) The wording of this error message may depend on details of Clang and/or libc++; does that mean this test needs to go in libcxx/test/libcxx/? If so, then it might be better simply to write a compile-only (non-fail) test that just does
```
static_assert(!std::is_convertible_v<decltype(nullptr), std::string>);
static_assert(!std::is_constructible_v<std::string, decltype(nullptr)>);
static_assert(!std::is_assignable_v<std::string, decltype(nullptr)>);
```
and leave it at that.

I was going to suggest an overload-resolution test like
```
void f(std::unique_ptr<int>) { }
void f(const std::string&);
int main() { f(nullptr); }
```
which I'm sure programmers would //like// to Just Work... but [p2166] deliberately keeps that as ambiguous. (The user-defined conversion from `nullptr` to `std::string` ranks lower in preference than a standard conversion such as `nullptr`-to-`int*`, but delete'ing it doesn't make it go away; it just says "The API designer knows what you're trying to do, and you're wrong to try it." So we still have an ambiguity here between the right thing (conversion to `unique_ptr`) and the wrong thing (conversion to `string`), with no tiebreaker for picking between them.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106801