diff mbox series

libstdc++: Set active union member in constexpr std::string [PR103295]

Message ID Y24wszWBpJVRv1ma@Thaum.localdomain
State Accepted
Headers
Series libstdc++: Set active union member in constexpr std::string [PR103295] |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Nathaniel Shead Nov. 11, 2022, 11:23 a.m. UTC 
  Hi,

Below is a patch to fix std::string in constexpr contexts on Clang. This
was originally fixed in the commits attached to PR103295, but a later
commit 98a0d72a seems to have mistakenly undone this.

Tested on x86_64-linux. Verified using clang-14 and clang-15 that the
fix works. I haven't added anything to the test suite, since this issue
is only detected by clang.

This is my first time contributing, so please let me know if I've done
anything wrong or missed something. Thanks!

Nathaniel

-- >8 --

Clang still complains about using std::string in constexpr contexts due
to the changes made in commit 98a0d72a. This patch ensures that we set
the active member of the union as according to [class.union.general] p6.

libstdc++-v3/ChangeLog:

	PR libstdc++/103295
        * include/bits/basic_string.h (_M_use_local_data): Set active
          member to _M_local_buf.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
---
 libstdc++-v3/include/bits/basic_string.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jonathan Wakely Nov. 11, 2022, 5:12 p.m. UTC | #1 
On Fri, 11 Nov 2022 at 11:23, Nathaniel Shead via Libstdc++
<libstdc++@gcc.gnu.org> wrote:
>
> Hi,
>
> Below is a patch to fix std::string in constexpr contexts on Clang. This
> was originally fixed in the commits attached to PR103295, but a later
> commit 98a0d72a seems to have mistakenly undone this.
>
> Tested on x86_64-linux. Verified using clang-14 and clang-15 that the
> fix works. I haven't added anything to the test suite, since this issue
> is only detected by clang.
>
> This is my first time contributing, so please let me know if I've done
> anything wrong or missed something. Thanks!

Thanks for the patch, I'll get this committed today.

The only thing I had to fix was the indentation in the commit log. The
second line of the ChangeLog should be aligned with the * not the text
following it (so indented by a single tab).


>
> Nathaniel
>
> -- >8 --
>
> Clang still complains about using std::string in constexpr contexts due
> to the changes made in commit 98a0d72a. This patch ensures that we set
> the active member of the union as according to [class.union.general] p6.
>
> libstdc++-v3/ChangeLog:
>
>         PR libstdc++/103295
>         * include/bits/basic_string.h (_M_use_local_data): Set active
>           member to _M_local_buf.
>
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>  libstdc++-v3/include/bits/basic_string.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
> index 9c2b57f5a1d..2790fd49b05 100644
> --- a/libstdc++-v3/include/bits/basic_string.h
> +++ b/libstdc++-v3/include/bits/basic_string.h
> @@ -352,8 +352,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
>        {
>  #if __cpp_lib_is_constant_evaluated
>         if (std::is_constant_evaluated())
> -         for (_CharT& __c : _M_local_buf)
> -           __c = _CharT();
> +         for (size_type i = 0; i <= _S_local_capacity; ++i)
> +           _M_local_buf[i] = _CharT();
>  #endif
>         return _M_local_data();
>        }
> --
> 2.34.1
>
Patrick Palka Nov. 11, 2022, 5:55 p.m. UTC | #2 
On Fri, 11 Nov 2022, Jonathan Wakely via Libstdc++ wrote:

> On Fri, 11 Nov 2022 at 11:23, Nathaniel Shead via Libstdc++
> <libstdc++@gcc.gnu.org> wrote:
> >
> > Hi,
> >
> > Below is a patch to fix std::string in constexpr contexts on Clang. This
> > was originally fixed in the commits attached to PR103295, but a later
> > commit 98a0d72a seems to have mistakenly undone this.
> >
> > Tested on x86_64-linux. Verified using clang-14 and clang-15 that the
> > fix works. I haven't added anything to the test suite, since this issue
> > is only detected by clang.
> >
> > This is my first time contributing, so please let me know if I've done
> > anything wrong or missed something. Thanks!
> 
> Thanks for the patch, I'll get this committed today.
> 
> The only thing I had to fix was the indentation in the commit log. The
> second line of the ChangeLog should be aligned with the * not the text
> following it (so indented by a single tab).
> 
> 
> >
> > Nathaniel
> >
> > -- >8 --
> >
> > Clang still complains about using std::string in constexpr contexts due
> > to the changes made in commit 98a0d72a. This patch ensures that we set
> > the active member of the union as according to [class.union.general] p6.
> >
> > libstdc++-v3/ChangeLog:
> >
> >         PR libstdc++/103295
> >         * include/bits/basic_string.h (_M_use_local_data): Set active
> >           member to _M_local_buf.
> >
> > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > ---
> >  libstdc++-v3/include/bits/basic_string.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
> > index 9c2b57f5a1d..2790fd49b05 100644
> > --- a/libstdc++-v3/include/bits/basic_string.h
> > +++ b/libstdc++-v3/include/bits/basic_string.h
> > @@ -352,8 +352,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> >        {
> >  #if __cpp_lib_is_constant_evaluated
> >         if (std::is_constant_evaluated())
> > -         for (_CharT& __c : _M_local_buf)
> > -           __c = _CharT();
> > +         for (size_type i = 0; i <= _S_local_capacity; ++i)
> > +           _M_local_buf[i] = _CharT();

Just a minor nit, but we should probably uglify i to __i here.

> >  #endif
> >         return _M_local_data();
> >        }
> > --
> > 2.34.1
> >
> 
>
Jonathan Wakely Nov. 11, 2022, 5:59 p.m. UTC | #3 
On Fri, 11 Nov 2022 at 17:55, Patrick Palka <ppalka@redhat.com> wrote:
>
> On Fri, 11 Nov 2022, Jonathan Wakely via Libstdc++ wrote:
>
> > On Fri, 11 Nov 2022 at 11:23, Nathaniel Shead via Libstdc++
> > <libstdc++@gcc.gnu.org> wrote:
> > >
> > > Hi,
> > >
> > > Below is a patch to fix std::string in constexpr contexts on Clang. This
> > > was originally fixed in the commits attached to PR103295, but a later
> > > commit 98a0d72a seems to have mistakenly undone this.
> > >
> > > Tested on x86_64-linux. Verified using clang-14 and clang-15 that the
> > > fix works. I haven't added anything to the test suite, since this issue
> > > is only detected by clang.
> > >
> > > This is my first time contributing, so please let me know if I've done
> > > anything wrong or missed something. Thanks!
> >
> > Thanks for the patch, I'll get this committed today.
> >
> > The only thing I had to fix was the indentation in the commit log. The
> > second line of the ChangeLog should be aligned with the * not the text
> > following it (so indented by a single tab).
> >
> >
> > >
> > > Nathaniel
> > >
> > > -- >8 --
> > >
> > > Clang still complains about using std::string in constexpr contexts due
> > > to the changes made in commit 98a0d72a. This patch ensures that we set
> > > the active member of the union as according to [class.union.general] p6.
> > >
> > > libstdc++-v3/ChangeLog:
> > >
> > >         PR libstdc++/103295
> > >         * include/bits/basic_string.h (_M_use_local_data): Set active
> > >           member to _M_local_buf.
> > >
> > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > > ---
> > >  libstdc++-v3/include/bits/basic_string.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
> > > index 9c2b57f5a1d..2790fd49b05 100644
> > > --- a/libstdc++-v3/include/bits/basic_string.h
> > > +++ b/libstdc++-v3/include/bits/basic_string.h
> > > @@ -352,8 +352,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> > >        {
> > >  #if __cpp_lib_is_constant_evaluated
> > >         if (std::is_constant_evaluated())
> > > -         for (_CharT& __c : _M_local_buf)
> > > -           __c = _CharT();
> > > +         for (size_type i = 0; i <= _S_local_capacity; ++i)
> > > +           _M_local_buf[i] = _CharT();
>
> Just a minor nit, but we should probably uglify i to __i here.

Good catch, thanks. Fixed and pushed.
Nathaniel Shead Nov. 12, 2022, 12:41 a.m. UTC | #4 
Thanks for that. I'll keep your comments in mind for the future.

On Fri, Nov 11, 2022 at 05:59:33PM +0000, Jonathan Wakely wrote:
> On Fri, 11 Nov 2022 at 17:55, Patrick Palka <ppalka@redhat.com> wrote:
> >
> > On Fri, 11 Nov 2022, Jonathan Wakely via Libstdc++ wrote:
> >
> > > On Fri, 11 Nov 2022 at 11:23, Nathaniel Shead via Libstdc++
> > > <libstdc++@gcc.gnu.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > Below is a patch to fix std::string in constexpr contexts on Clang. This
> > > > was originally fixed in the commits attached to PR103295, but a later
> > > > commit 98a0d72a seems to have mistakenly undone this.
> > > >
> > > > Tested on x86_64-linux. Verified using clang-14 and clang-15 that the
> > > > fix works. I haven't added anything to the test suite, since this issue
> > > > is only detected by clang.
> > > >
> > > > This is my first time contributing, so please let me know if I've done
> > > > anything wrong or missed something. Thanks!
> > >
> > > Thanks for the patch, I'll get this committed today.
> > >
> > > The only thing I had to fix was the indentation in the commit log. The
> > > second line of the ChangeLog should be aligned with the * not the text
> > > following it (so indented by a single tab).
> > >
> > >
> > > >
> > > > Nathaniel
> > > >
> > > > -- >8 --
> > > >
> > > > Clang still complains about using std::string in constexpr contexts due
> > > > to the changes made in commit 98a0d72a. This patch ensures that we set
> > > > the active member of the union as according to [class.union.general] p6.
> > > >
> > > > libstdc++-v3/ChangeLog:
> > > >
> > > >         PR libstdc++/103295
> > > >         * include/bits/basic_string.h (_M_use_local_data): Set active
> > > >           member to _M_local_buf.
> > > >
> > > > Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> > > > ---
> > > >  libstdc++-v3/include/bits/basic_string.h | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
> > > > index 9c2b57f5a1d..2790fd49b05 100644
> > > > --- a/libstdc++-v3/include/bits/basic_string.h
> > > > +++ b/libstdc++-v3/include/bits/basic_string.h
> > > > @@ -352,8 +352,8 @@ _GLIBCXX_BEGIN_NAMESPACE_CXX11
> > > >        {
> > > >  #if __cpp_lib_is_constant_evaluated
> > > >         if (std::is_constant_evaluated())
> > > > -         for (_CharT& __c : _M_local_buf)
> > > > -           __c = _CharT();
> > > > +         for (size_type i = 0; i <= _S_local_capacity; ++i)
> > > > +           _M_local_buf[i] = _CharT();
> >
> > Just a minor nit, but we should probably uglify i to __i here.
> 
> Good catch, thanks. Fixed and pushed.
>
diff mbox series

Patch

diff --git a/libstdc++-v3/include/bits/basic_string.h b/libstdc++-v3/include/bits/basic_string.h
index 9c2b57f5a1d..2790fd49b05 100644
--- a/libstdc++-v3/include/bits/basic_string.h
+++ b/libstdc++-v3/include/bits/basic_string.h
@@ -352,8 +352,8 @@  _GLIBCXX_BEGIN_NAMESPACE_CXX11
       {
 #if __cpp_lib_is_constant_evaluated
 	if (std::is_constant_evaluated())
-	  for (_CharT& __c : _M_local_buf)
-	    __c = _CharT();
+	  for (size_type i = 0; i <= _S_local_capacity; ++i)
+	    _M_local_buf[i] = _CharT();
 #endif
 	return _M_local_data();
       }