[libcxx-commits] [PATCH] D108906: Add std::assume_aligned

[Louis Dionne via Phabricator via libcxx-commits]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

Thanks for the patch!

Can you follow the check-list at https://libcxx.llvm.org/Contributing.html (re-generating auto-gen-files, updating status pages, updating synopses, etc.)?



================
Comment at: libcxx/include/memory:959
+[[nodiscard]]
+_LIBCPP_CONSTEXPR
+_LIBCPP_ALWAYS_INLINE
----------------
This can be just `constexpr` since it's C++20-and-above.


================
Comment at: libcxx/include/memory:960
+_LIBCPP_CONSTEXPR
+_LIBCPP_ALWAYS_INLINE
+T* assume_aligned(T* __ptr)
----------------
This should be `_LIBCPP_HIDE_FROM_ABI`.


================
Comment at: libcxx/include/memory:961
+_LIBCPP_ALWAYS_INLINE
+T* assume_aligned(T* __ptr)
+#if defined(__clang__)
----------------
Please move this to a new header `__memory/assumed_aligned.h`.


================
Comment at: libcxx/test/std/utilities/memory/ptr.align/assume_aligned.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Can you please rename this to `assume_alligned.verify.cpp`?


================
Comment at: libcxx/test/std/utilities/memory/ptr.align/assume_aligned.pass.cpp:18-22
+constexpr int test() {
+  int a;
+  [[maybe_unused]] int* b = std::assume_aligned<4>(&a);
+  return sizeof(int);
+}
----------------
We generally follow this pattern:

```
constexpr bool test() {
  // ... tests go here

  return true;
}

int main(int, char**) {
  test();
  static_assert(test());

  return 0;
}
```

As to the tests themselves, can you look at https://reviews.llvm.org/D54966 and grab what's relevant? I'm seeing a lot more tests in https://reviews.llvm.org/D54966. For example, we should at least test that `assume_aligned` returns the same pointer that it is being passed. We should also test with a couple different types, etc.


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

https://reviews.llvm.org/D108906