[libcxx] removes unnecessary traits from has_unique_object_representations

[cjdb]

remove_cv_t and remove_all_extents_t are taken care of by the built-in trait, so we don't need to use them directly.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Christopher Di Bella (cjdb)

Changes

remove_cv_t and remove_all_extents_t are taken care of by the built-in trait, so we don't need to use them directly.

---

Full diff: https://github.com/llvm/llvm-project/pull/69241.diff

1 Files Affected:

• (modified) libcxx/include/__type_traits/has_unique_object_representation.h (+1-3)

diff --git a/libcxx/include/__type_traits/has_unique_object_representation.h b/libcxx/include/__type_traits/has_unique_object_representation.h
index c0ada5618f0e3ed..0e90193df9d4348 100644
--- a/libcxx/include/__type_traits/has_unique_object_representation.h
+++ b/libcxx/include/__type_traits/has_unique_object_representation.h
@@ -11,8 +11,6 @@
 
 #include <__config>
 #include <__type_traits/integral_constant.h>
-#include <__type_traits/remove_all_extents.h>
-#include <__type_traits/remove_cv.h>
 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
@@ -24,7 +22,7 @@ _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp>
 struct _LIBCPP_TEMPLATE_VIS has_unique_object_representations
-    : public integral_constant<bool, __has_unique_object_representations(remove_cv_t<remove_all_extents_t<_Tp>>)> {};
+    : public integral_constant<bool, __has_unique_object_representations(_Tp)> {};
 
 template <class _Tp>
 inline constexpr bool has_unique_object_representations_v = has_unique_object_representations<_Tp>::value;

[EricWF]

Are we sure what the behavior is here with GCC?

LGTM after ensuring we have test coverage for the remove_cv/remove_ext bits.

[EricWF]

I assume you've checked that we have tests for const arrays or w/e to make sure the patch is still exercised.

[cjdb]

Yes to both questions, but I wouldn't be upset if someone wanted to double check the tests' existence.

[ldionne]

There is libcxx/test/std/utilities/meta/meta.unary/meta.unary.prop/has_unique_object_representations.pass.cpp, but we seem to be missing a test for a multi-dimensional array.

[philnik777]

Suggested change

	inline constexpr bool has_unique_object_representations_v = has_unique_object_representations<_Tp>::value;
	inline constexpr bool has_unique_object_representations_v = __has_unique_object_representations(_Tp);

Might as well while we're here.

[cjdb]

Very happy to do this if we're okay with doing it elsewhere (I can lead the clean-up for that).

[philnik777]

Depends on what you mean by "it". If you just mean cases where we always have the builtins available, sure (although I think I removed those cases). In other cases I'm not sure it's a good idea.

[cjdb]

I mean applying the same change to all other traits where the trait is how the builtin is surfaced (so mostly stuff in <type_traits>). Anything else deserves a proper discussion.

[ldionne]

I think what Nikolas tried explaining is that in most cases, we already call the builtin directly from the _v alias if there's a builtin (for example libcxx/include/__type_traits/is_reference.h). So I think there should be no other places where we want to make the same change, unless of course we forgot to do it in other places.

[ldionne]

Gentle ping on this. I think the remaining changes needed are:

1. Add a test for multidimensional arrays in has_unique_object_representation (so that we test the remove_all_extents bit)
2. Also use __has_unique_object_representations for has_unique_object_representations_v

Apart from that, I think everyone was happy with the patch.

[philnik777]

@cjdb is anything else required for this patch to land?

[ldionne]

This can be merged once the CI is green. Whoever sees this first can go ahead.

[amilendra]

Hello @philnik777
This change seems to have ( I think ) unintentionally changed the behavior of the following program.
Would you please check and fix if you agree with this analysis?

#include <type_traits>

template <int>
class class_type {
    int i;
};
typedef class_type<1> T;

static_assert(std::has_unique_object_representations<T[][2][3]>::value);

https://gcc.godbolt.org/z/Ejo8z61Td

[ldionne]

@amilendra Thanks for the report, see #95314 and #95311.