Re: [PATCH] libstdc++: Allow adjacent __maybe_present_t<false, ...> to overlap

Re: [PATCH] libstdc++: Allow adjacent __maybe_present_t<false, ...> to overlap

[Pilar Latiesa]

[-- Attachment #1: Type: text/plain, Size: 308 bytes --]

Just out of curiosity: would this also work?

template<typename _Tp, typename _Disc>
struct _Absent {};

template<bool _Present, typename _Tp, typename _Disc = decltype([]{})>
using __maybe_present_t = __conditional_t<_Present, _Tp, _Absent<_Tp,
_Disc>>;

That would avoid having to type 0, 1, ... manually.

Re: [PATCH] libstdc++: Allow adjacent __maybe_present_t<false, ...> to overlap

[Jonathan Wakely]

On Tue, 2 Apr 2024 at 18:00, Pilar Latiesa wrote:
>
> Just out of curiosity: would this also work?
>
> template<typename _Tp, typename _Disc>
> struct _Absent {};
>
> template<bool _Present, typename _Tp, typename _Disc = decltype([]{})>
> using __maybe_present_t = __conditional_t<_Present, _Tp, _Absent<_Tp, _Disc>>;
>
> That would avoid having to type 0, 1, ... manually.

This is subjectively horrible and, more objectively, would create
longer mangled names and additional RTTI.

Re: [PATCH] libstdc++: Allow adjacent __maybe_present_t<false, ...> to overlap

[Patrick Palka]

On Tue, 2 Apr 2024, Jonathan Wakely wrote:

> On Tue, 2 Apr 2024 at 18:00, Pilar Latiesa wrote:
> >
> > Just out of curiosity: would this also work?
> >
> > template<typename _Tp, typename _Disc>
> > struct _Absent {};
> >
> > template<bool _Present, typename _Tp, typename _Disc = decltype([]{})>
> > using __maybe_present_t = __conditional_t<_Present, _Tp, _Absent<_Tp, _Disc>>;
> >
> > That would avoid having to type 0, 1, ... manually.
> 
> This is subjectively horrible and, more objectively, would create
> longer mangled names and additional RTTI.

Yeah, it's a neat trick but probably not appropriate to use within the
standard library.

Another reason to avoid it is that GCC's support for lambdas within
template arguments has some known bugs (e.g. PR107457 but that should
hopefully be fixed soon).

Re: [PATCH] libstdc++: Allow adjacent __maybe_present_t<false, ...> to overlap

[Pilar Latiesa]

> > This is subjectively horrible and, more objectively, would create
> > longer mangled names and additional RTTI.

> Yeah, it's a neat trick but probably not appropriate to use within the
> standard library.

I understand. I was genuinely curious about whether this would do the trick.

In fact, if I'm not mistaken, the whole thing could be simplified to:

template<bool _Present, typename _Tp, typename _Absent = decltype([]{})>
using __maybe_present_t = __conditional_t<_Present, _Tp, _Absent>;

[PATCH] libstdc++: Allow adjacent __maybe_present_t<false, ...> to overlap

[Patrick Palka]

Tested on x86_64-pc-linux-gnu, does this look OK for trunk?

-- >8 --

Currently __maybe_present_t<false, T> maps to the same empty class
type independent of T.  This is suboptimal because it means adjacent
__maybe_present_t<false, ...> members with the [[no_unique_address]]
attribute can't overlap even if the conditionally present types are
different.

This patch fixes this by turning this empty class type into a template
parameterized by the conditionally present type, so that

  [[no_unique_address]] __maybe_present_t<false, T> _M_a;
  [[no_unique_address]] __maybe_present_t<false, U> _M_b;

now overlap if T and U are different.

This patch goes a step further and also adds an optional integer
discriminator parameter to allow for overlapping when T and U are
the same.

libstdc++-v3/ChangeLog:

	* include/std/ranges (ranges::__detail::_Empty): Rename to ...
	(ranges::__detail::_Absent): ... this.  Turn into a template
	parameterized by the absent type _Tp and discriminator _Disc.
	(ranges::__detail::__maybe_present_t): Add an optional
	discriminator parameter.
	(slide_view::_M_cached_begin): Pass a discriminator argument to
	__maybe_present_t.
	(slide_view::_M_cached_end): Likewise.
	* testsuite/std/ranges/adaptors/sizeof.cc: Verify the size of
	slide_view<V> is 3 instead 4 pointers.
---
 libstdc++-v3/include/std/ranges                     | 13 ++++++++-----
 .../testsuite/std/ranges/adaptors/sizeof.cc         |  4 ++++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
index 7d739852677..afce818376b 100644
--- a/libstdc++-v3/include/std/ranges
+++ b/libstdc++-v3/include/std/ranges
@@ -886,14 +886,17 @@ namespace views
 
 namespace __detail
 {
-  struct _Empty { };
+  template<typename _Tp, int _Disc>
+    struct _Absent { };
 
   // Alias for a type that is conditionally present
   // (and is an empty type otherwise).
   // Data members using this alias should use [[no_unique_address]] so that
   // they take no space when not needed.
-  template<bool _Present, typename _Tp>
-    using __maybe_present_t = __conditional_t<_Present, _Tp, _Empty>;
+  // The optional template parameter _Disc is for discriminating two otherwise
+  // equivalent absent types so that even they can overlap.
+  template<bool _Present, typename _Tp, int _Disc = 0>
+    using __maybe_present_t = __conditional_t<_Present, _Tp, _Absent<_Tp, _Disc>>;
 
   // Alias for a type that is conditionally const.
   template<bool _Const, typename _Tp>
@@ -6553,10 +6556,10 @@ namespace views::__adaptor
     range_difference_t<_Vp> _M_n;
     [[no_unique_address]]
       __detail::__maybe_present_t<__detail::__slide_caches_first<_Vp>,
-				  __detail::_CachedPosition<_Vp>> _M_cached_begin;
+				  __detail::_CachedPosition<_Vp>, 0> _M_cached_begin;
     [[no_unique_address]]
       __detail::__maybe_present_t<__detail::__slide_caches_last<_Vp>,
-				  __detail::_CachedPosition<_Vp>> _M_cached_end;
+				  __detail::_CachedPosition<_Vp>, 1> _M_cached_end;
 
     template<bool> class _Iterator;
     class _Sentinel;
diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/sizeof.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/sizeof.cc
index 12a9da3181d..08c01704d10 100644
--- a/libstdc++-v3/testsuite/std/ranges/adaptors/sizeof.cc
+++ b/libstdc++-v3/testsuite/std/ranges/adaptors/sizeof.cc
@@ -49,3 +49,7 @@ static_assert(sizeof(ranges::lazy_split_view<V, std::string_view>) == 4*ptr);
 
 static_assert
  (sizeof(ranges::reverse_view<ranges::filter_view<V, decltype(pred_l)>>) == 3*ptr);
+
+#if __cpp_lib_ranges_slide
+static_assert(sizeof(ranges::slide_view<V>) == 3*ptr);
+#endif
-- 
2.44.0.448.gc2cbfbd2e2

Re: [PATCH] libstdc++: Allow adjacent __maybe_present_t<false, ...> to overlap

[Jonathan Wakely]

On Mon, 1 Apr 2024 at 23:16, Patrick Palka <ppalka@redhat.com> wrote:
>
> Tested on x86_64-pc-linux-gnu, does this look OK for trunk?

This is a layout change for some specializations of slide_view, but
better to do that now than change it between gcc 14 and 15.

OK for trunk.


>
> -- >8 --
>
> Currently __maybe_present_t<false, T> maps to the same empty class
> type independent of T.  This is suboptimal because it means adjacent
> __maybe_present_t<false, ...> members with the [[no_unique_address]]
> attribute can't overlap even if the conditionally present types are
> different.
>
> This patch fixes this by turning this empty class type into a template
> parameterized by the conditionally present type, so that
>
>   [[no_unique_address]] __maybe_present_t<false, T> _M_a;
>   [[no_unique_address]] __maybe_present_t<false, U> _M_b;
>
> now overlap if T and U are different.
>
> This patch goes a step further and also adds an optional integer
> discriminator parameter to allow for overlapping when T and U are
> the same.
>
> libstdc++-v3/ChangeLog:
>
>         * include/std/ranges (ranges::__detail::_Empty): Rename to ...
>         (ranges::__detail::_Absent): ... this.  Turn into a template
>         parameterized by the absent type _Tp and discriminator _Disc.
>         (ranges::__detail::__maybe_present_t): Add an optional
>         discriminator parameter.
>         (slide_view::_M_cached_begin): Pass a discriminator argument to
>         __maybe_present_t.
>         (slide_view::_M_cached_end): Likewise.
>         * testsuite/std/ranges/adaptors/sizeof.cc: Verify the size of
>         slide_view<V> is 3 instead 4 pointers.
> ---
>  libstdc++-v3/include/std/ranges                     | 13 ++++++++-----
>  .../testsuite/std/ranges/adaptors/sizeof.cc         |  4 ++++
>  2 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/libstdc++-v3/include/std/ranges b/libstdc++-v3/include/std/ranges
> index 7d739852677..afce818376b 100644
> --- a/libstdc++-v3/include/std/ranges
> +++ b/libstdc++-v3/include/std/ranges
> @@ -886,14 +886,17 @@ namespace views
>
>  namespace __detail
>  {
> -  struct _Empty { };
> +  template<typename _Tp, int _Disc>
> +    struct _Absent { };
>
>    // Alias for a type that is conditionally present
>    // (and is an empty type otherwise).
>    // Data members using this alias should use [[no_unique_address]] so that
>    // they take no space when not needed.
> -  template<bool _Present, typename _Tp>
> -    using __maybe_present_t = __conditional_t<_Present, _Tp, _Empty>;
> +  // The optional template parameter _Disc is for discriminating two otherwise
> +  // equivalent absent types so that even they can overlap.
> +  template<bool _Present, typename _Tp, int _Disc = 0>
> +    using __maybe_present_t = __conditional_t<_Present, _Tp, _Absent<_Tp, _Disc>>;
>
>    // Alias for a type that is conditionally const.
>    template<bool _Const, typename _Tp>
> @@ -6553,10 +6556,10 @@ namespace views::__adaptor
>      range_difference_t<_Vp> _M_n;
>      [[no_unique_address]]
>        __detail::__maybe_present_t<__detail::__slide_caches_first<_Vp>,
> -                                 __detail::_CachedPosition<_Vp>> _M_cached_begin;
> +                                 __detail::_CachedPosition<_Vp>, 0> _M_cached_begin;
>      [[no_unique_address]]
>        __detail::__maybe_present_t<__detail::__slide_caches_last<_Vp>,
> -                                 __detail::_CachedPosition<_Vp>> _M_cached_end;
> +                                 __detail::_CachedPosition<_Vp>, 1> _M_cached_end;
>
>      template<bool> class _Iterator;
>      class _Sentinel;
> diff --git a/libstdc++-v3/testsuite/std/ranges/adaptors/sizeof.cc b/libstdc++-v3/testsuite/std/ranges/adaptors/sizeof.cc
> index 12a9da3181d..08c01704d10 100644
> --- a/libstdc++-v3/testsuite/std/ranges/adaptors/sizeof.cc
> +++ b/libstdc++-v3/testsuite/std/ranges/adaptors/sizeof.cc
> @@ -49,3 +49,7 @@ static_assert(sizeof(ranges::lazy_split_view<V, std::string_view>) == 4*ptr);
>
>  static_assert
>   (sizeof(ranges::reverse_view<ranges::filter_view<V, decltype(pred_l)>>) == 3*ptr);
> +
> +#if __cpp_lib_ranges_slide
> +static_assert(sizeof(ranges::slide_view<V>) == 3*ptr);
> +#endif
> --
> 2.44.0.448.gc2cbfbd2e2
>