[libcxx-commits] [PATCH] D136724: [libc++][RFC] Add nullability annotations to string

[Eric Fiselier via Phabricator via libcxx-commits]

EricWF requested changes to this revision.
EricWF added a comment.

I'm not sure how much value the `_Null_unspecified` and `_Nullable` annotations add.
I also don't see the value in annotating private internals, presumably we're not passing bad data ourselves, and any bad user data should be caught as it enters via the public API.

Further, there are a lot of changes to `<string>` but almost no tests for them. For each application of the `_Nonnull` annotation, we should be able to write a test that demonstrates the behavior change. If the annotation cannot be observed, we shouldn't add it.

Requesting more tests before this lands.



================
Comment at: libcxx/include/string:892
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const _CharT* __s, size_type __n)
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string(const _CharT* _Nullable __s, size_type __n)
       : __r_(__default_init_tag(), __default_init_tag()) {
----------------
What value is produced by adding `_Nullable' here?

I get adding `_Nonnull` where applicable, but I'm less sold on the rest of the annotations.


================
Comment at: libcxx/include/string:1763
     template <bool __is_short>
-    _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& __assign_no_alias(const value_type* __s, size_type __n);
+    _LIBCPP_CONSTEXPR_SINCE_CXX20 basic_string& __assign_no_alias(const value_type* _Null_unspecified __s, size_type __n);
 
----------------
Do we really need to decorate internal functions? We should be able to diagnose all ill-formed user data when it's passed into the public API.




================
Comment at: libcxx/test/libcxx/strings/basic.string/string.cons/ptr.nonnull.verify.cpp:14
+void func() {
+  std::string s(static_cast<char*>(nullptr)); // Null passed to a callee that requires a non-null argument
+}
----------------
Does this pass? It doesn't look like your typical verify tests. I think there's something going on here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136724