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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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}}
+
----------------
Quuxplusone wrote:
> (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.)
I would go for the `static_assert` tests in a `.compile.pass.cpp` test file too. All in the same file IMO.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106801