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

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/include/__config:1093
+#  if __has_attribute(__no_sanitize__)
+#    define _LIBCPP_NO_SANITIZE(...) __attribute__((__no_sanitize__(__VA_ARGS__)))
+#  else
----------------
philnik wrote:
> 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.
I think it would help readers to quickly find the documentation, instead of remember where the clang attributes are exactly available.


================
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");
+
----------------
philnik wrote:
> 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.
Yes a range is `[begin, end)`, and an empty for an empty range `begin == end`.
I'm happy with the fix.


================
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;
----------------
philnik wrote:
> Mordante wrote:
> > Is this really safe on all supported platforms?
> I think it should be. Do you have anything specific in mind?
You can compare integral types with different sizes, but you can't just cast pointers from one to another.

Concretely I wonder whether we violate alignment rules.
For example`_Tp == int`, `_Up = signed char` and `__ptr = 0xFFF01`. This will create a `const int*` at an odd numbered address. (This is valid on x86, but that platform has less alignment restrictions than other platforms.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143327