[libcxx-commits] [PATCH] D143327: [libc++] Introduce __is_pointer_in_range

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added a comment.

Note I fixed a typo in the commit message.

Thanks for working on this, I think it's useful to have, but it requires a bit more work.



================
Comment at: libcxx/include/CMakeLists.txt:716
   __utility/in_place.h
+  __utility/is_pointer_in_range.h
   __utility/integer_sequence.h
----------------
I miss the module map entry.


================
Comment at: libcxx/include/__config:1093
+#  if __has_attribute(__no_sanitize__)
+#    define _LIBCPP_NO_SANITIZE(...) __attribute__((__no_sanitize__(__VA_ARGS__)))
+#  else
----------------
Can you add a link to the documentation of this attribute?


================
Comment at: libcxx/include/__utility/is_pointer_in_range.h:33
+  if (__libcpp_is_constant_evaluated()) {
+    _LIBCPP_ASSERT(__builtin_constant_p(__begin < __end), "__begin and __end do not form a range");
+
----------------
Or are empty ranges prohibited?

Based on the code below it is indeed prohibited. If that is intended I really dislike the name of the function.
The function does not test whether the pointer is in the range, but whether the pointer is in the range and can be dereferenced.Maybe `__is_pointer_in_dereferencable_range` would be a better name.


================
Comment at: libcxx/include/__utility/is_pointer_in_range.h:41
+
+  return __begin <= __ptr && __ptr < __end;
+}
----------------
I think we should document this is technically UB, but all supported compilers accept this.


================
Comment at: libcxx/include/__utility/is_pointer_in_range.h:50
+
+  auto __t_ptr = reinterpret_cast<const _Tp*>(__ptr);
+  return __begin <= __t_ptr && __t_ptr < __end;
----------------
Is this really safe on all supported platforms?


================
Comment at: libcxx/test/libcxx/utilities/is_pointer_in_range.pass.cpp:37
+
+int main(int, char**) {
+  test();
----------------
I really like to see more test. This only compares pointers where `_Tp` and `_Up` are the same type. Since the code tests different pointer types I really would like to see tests for `volatile` pointers against `non-volatile` pointers. Test with object pointers against function pointers, data member pointer, member function pointers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143327