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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__config:1093
+#  if __has_attribute(__no_sanitize__)
+#    define _LIBCPP_NO_SANITIZE(...) __attribute__((__no_sanitize__(__VA_ARGS__)))
+#  else
----------------
Mordante wrote:
> Can you add a link to the documentation of this attribute?
I don't really see the point here. All the clang attributes are described at https://clang.llvm.org/docs/AttributeReference.html. Having a link to the same page for every attribute we use seems redundant. We can add a link at the top of the file if you think that's useful.


================
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");
+
----------------
Mordante wrote:
> 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.
No, empty ranges are not prohibited. This just checks that it is valid to compare the two pointers. It doesn't actually check that `__begin` is smaller than `__end`. IOW `__builtin_constant_p(ptr < ptr)` is always true, since the expression is known to result in `false` (https://godbolt.org/z/EMfW7179r).

AFAIK a range is normally [begin, end), i.e. excluding the end pointer. Returning true for the end pointer would be inconsistent with any other function taking a range.


================
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;
----------------
Mordante wrote:
> Is this really safe on all supported platforms?
I think it should be. Do you have anything specific in mind?


================
Comment at: libcxx/test/libcxx/utilities/is_pointer_in_range.pass.cpp:37
+
+int main(int, char**) {
+  test();
----------------
Mordante wrote:
> 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.
I don't think it makes sense to call `__is_pointer_in_range` with function pointers or member pointers, so I added `static_assert`s instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143327