[PATCH] Use std::optional when available

[PATCH] Use std::optional when available

[Tom Tromey]

This patch changes gdb_optional.h to use std::optional when it is
available.

Note I had to comment out some of the tests.  These give a
set-but-not-used warning when built with the libstdc++ optional.  I
think this change is harmless, particularly because these tests will
be removed entirely once gdb switches to C++17.

Regression tested on x86-64 Fedora 36.
---
 gdb/unittests/optional-selftests.c  |  2 ++
 gdb/unittests/optional/cons/copy.cc |  4 ++++
 gdb/unittests/optional/cons/move.cc |  4 ++++
 gdbsupport/gdb_optional.h           | 17 +++++++++++++++++
 4 files changed, 27 insertions(+)

diff --git a/gdb/unittests/optional-selftests.c b/gdb/unittests/optional-selftests.c
index 8a727c02159..ea8447f4454 100644
--- a/gdb/unittests/optional-selftests.c
+++ b/gdb/unittests/optional-selftests.c
@@ -30,9 +30,11 @@
 /* libstdc++'s testsuite uses VERIFY.  */
 #define VERIFY SELF_CHECK
 
+#if __cplusplus < 201703L
 /* Used to disable testing features not supported by
    gdb::optional.  */
 #define GDB_OPTIONAL
+#endif
 
 namespace selftests {
 namespace optional {
diff --git a/gdb/unittests/optional/cons/copy.cc b/gdb/unittests/optional/cons/copy.cc
index 87a08f9a52b..4b9837dd3c5 100644
--- a/gdb/unittests/optional/cons/copy.cc
+++ b/gdb/unittests/optional/cons/copy.cc
@@ -89,6 +89,9 @@ test ()
 
   enum outcome { nothrow, caught, bad_catch };
 
+  /* These tests give an unused-but-set warning with libstdc++
+     optional.  */
+#ifdef GDB_OPTIONAL
   {
     outcome result = nothrow;
     gdb::optional<throwing_copy> o;
@@ -120,6 +123,7 @@ test ()
 
     VERIFY( result == caught );
   }
+#endif
 
   VERIFY( tracker::count == 0 );
 }
diff --git a/gdb/unittests/optional/cons/move.cc b/gdb/unittests/optional/cons/move.cc
index 398784ae7ec..072c1b83b97 100644
--- a/gdb/unittests/optional/cons/move.cc
+++ b/gdb/unittests/optional/cons/move.cc
@@ -87,6 +87,9 @@ test ()
 
   enum outcome { nothrow, caught, bad_catch };
 
+  /* These tests give an unused-but-set warning with libstdc++
+     optional.  */
+#ifdef GDB_OPTIONAL
   {
     outcome result = nothrow;
     gdb::optional<throwing_move> o;
@@ -118,6 +121,7 @@ test ()
 
     VERIFY( result == caught );
   }
+#endif
 
   VERIFY( tracker::count == 0 );
 }
diff --git a/gdbsupport/gdb_optional.h b/gdbsupport/gdb_optional.h
index 9b7b7b2f7f4..c177691bb65 100644
--- a/gdbsupport/gdb_optional.h
+++ b/gdbsupport/gdb_optional.h
@@ -20,6 +20,21 @@
 #ifndef COMMON_GDB_OPTIONAL_H
 #define COMMON_GDB_OPTIONAL_H
 
+#if __cplusplus >= 201703L
+
+#include <optional>
+
+namespace gdb
+{
+template<typename T> using optional = std::optional<T>;
+
+using in_place_t = std::in_place_t;
+
+inline constexpr in_place_t in_place{};
+}
+
+#else /* __cplusplus < 201703L  */
+
 #include "gdbsupport/traits.h"
 
 namespace gdb
@@ -230,4 +245,6 @@ class optional
 
 }
 
+#endif /* __cplusplus < 201703L */
+
 #endif /* COMMON_GDB_OPTIONAL_H */
-- 
2.40.1

Re: [PATCH] Use std::optional when available

[Andrew Burgess]

Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

> This patch changes gdb_optional.h to use std::optional when it is
> available.
>
> Note I had to comment out some of the tests.  These give a
> set-but-not-used warning when built with the libstdc++ optional.  I
> think this change is harmless, particularly because these tests will
> be removed entirely once gdb switches to C++17.
>
> Regression tested on x86-64 Fedora 36.

I think this change needs to be held off until we move to C++17 - at
which point gdb::optional can just be dropped.

My reasoning is this comment in gdb_optional.h:

  /* This class attempts to be a compatible subset of std::optional,
     which is slated to be available in C++17.  This class optionally
     holds an object of some type -- by default it is constructed not
     holding an object, but later the object can be "emplaced".  This is
     similar to using std::unique_ptr, but in-object allocation is
     guaranteed.
  
     Unlike std::optional, we currently only support copy/move
     construction/assignment of an optional<T> from either exactly
     optional<T> or T.  I.e., we don't support copy/move
     construction/assignment from optional<U> or U, when U is a type
     convertible to T.  Making that work depending on the definitions of
     T and U is somewhat complicated, and currently the users of this
     class don't need it.  */

So, someone using a C++17 by default compiler will have access to a
wider range of std::optional functionality, and might write GDB code
that relies on that behaviour.

Then, someone using an older compiler will find that GDB no longer
compiles.

I agree with the overall goal of moving to C++17, and support that move,
I just don't think this change should go in ahead of the C++17 move.

> ---
>  gdb/unittests/optional-selftests.c  |  2 ++
>  gdb/unittests/optional/cons/copy.cc |  4 ++++
>  gdb/unittests/optional/cons/move.cc |  4 ++++
>  gdbsupport/gdb_optional.h           | 17 +++++++++++++++++
>  4 files changed, 27 insertions(+)
>
> diff --git a/gdb/unittests/optional-selftests.c b/gdb/unittests/optional-selftests.c
> index 8a727c02159..ea8447f4454 100644
> --- a/gdb/unittests/optional-selftests.c
> +++ b/gdb/unittests/optional-selftests.c
> @@ -30,9 +30,11 @@
>  /* libstdc++'s testsuite uses VERIFY.  */
>  #define VERIFY SELF_CHECK
>  
> +#if __cplusplus < 201703L

Ignoring my previous comment for a moment; we now rely on this version
number being defined the same in two files.  Would it be better to move
the '#define GDB_OPTIONAL' into gdb_optional.h?  Then we can be certain
that it will be defined correctly, and in sync with any future changes
made in that file?

Thanks,
Andrew

>  /* Used to disable testing features not supported by
>     gdb::optional.  */
>  #define GDB_OPTIONAL
> +#endif
>  
>  namespace selftests {
>  namespace optional {
> diff --git a/gdb/unittests/optional/cons/copy.cc b/gdb/unittests/optional/cons/copy.cc
> index 87a08f9a52b..4b9837dd3c5 100644
> --- a/gdb/unittests/optional/cons/copy.cc
> +++ b/gdb/unittests/optional/cons/copy.cc
> @@ -89,6 +89,9 @@ test ()
>  
>    enum outcome { nothrow, caught, bad_catch };
>  
> +  /* These tests give an unused-but-set warning with libstdc++
> +     optional.  */
> +#ifdef GDB_OPTIONAL
>    {
>      outcome result = nothrow;
>      gdb::optional<throwing_copy> o;
> @@ -120,6 +123,7 @@ test ()
>  
>      VERIFY( result == caught );
>    }
> +#endif
>  
>    VERIFY( tracker::count == 0 );
>  }
> diff --git a/gdb/unittests/optional/cons/move.cc b/gdb/unittests/optional/cons/move.cc
> index 398784ae7ec..072c1b83b97 100644
> --- a/gdb/unittests/optional/cons/move.cc
> +++ b/gdb/unittests/optional/cons/move.cc
> @@ -87,6 +87,9 @@ test ()
>  
>    enum outcome { nothrow, caught, bad_catch };
>  
> +  /* These tests give an unused-but-set warning with libstdc++
> +     optional.  */
> +#ifdef GDB_OPTIONAL
>    {
>      outcome result = nothrow;
>      gdb::optional<throwing_move> o;
> @@ -118,6 +121,7 @@ test ()
>  
>      VERIFY( result == caught );
>    }
> +#endif
>  
>    VERIFY( tracker::count == 0 );
>  }
> diff --git a/gdbsupport/gdb_optional.h b/gdbsupport/gdb_optional.h
> index 9b7b7b2f7f4..c177691bb65 100644
> --- a/gdbsupport/gdb_optional.h
> +++ b/gdbsupport/gdb_optional.h
> @@ -20,6 +20,21 @@
>  #ifndef COMMON_GDB_OPTIONAL_H
>  #define COMMON_GDB_OPTIONAL_H
>  
> +#if __cplusplus >= 201703L
> +
> +#include <optional>
> +
> +namespace gdb
> +{
> +template<typename T> using optional = std::optional<T>;
> +
> +using in_place_t = std::in_place_t;
> +
> +inline constexpr in_place_t in_place{};
> +}
> +
> +#else /* __cplusplus < 201703L  */
> +
>  #include "gdbsupport/traits.h"
>  
>  namespace gdb
> @@ -230,4 +245,6 @@ class optional
>  
>  }
>  
> +#endif /* __cplusplus < 201703L */
> +
>  #endif /* COMMON_GDB_OPTIONAL_H */
> -- 
> 2.40.1

Re: [PATCH] Use std::optional when available

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:

Andrew> I think this change needs to be held off until we move to C++17 - at
Andrew> which point gdb::optional can just be dropped.

Once we switch I think we can just remove this code entirely.

GDB 14 branched, so I think we're ready for that now.

>> +#if __cplusplus < 201703L

Andrew> Ignoring my previous comment for a moment; we now rely on this version
Andrew> number being defined the same in two files.  Would it be better to move
Andrew> the '#define GDB_OPTIONAL' into gdb_optional.h?  Then we can be certain
Andrew> that it will be defined correctly, and in sync with any future changes
Andrew> made in that file?

I don't think that would really help.  IIUC the issue would be compiling
two source files with different -std settings.  But in this case
gdb_optional.h would react to this.  Maybe I'm not understanding what
you mean though.

It seems to me that we simply don't need to support compiling the tree
with different std settings for different files -- it should be fine to
just assume that everything is compiled compatibly.

Tom

Re: [PATCH] Use std::optional when available

[Pedro Alves]

On 2023-10-10 17:49, Tom Tromey wrote:
>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
> 
> Andrew> I think this change needs to be held off until we move to C++17 - at
> Andrew> which point gdb::optional can just be dropped.
> 
> Once we switch I think we can just remove this code entirely.

Agreed, once we can rely on std::optional, we should delete all the tests
under gdb/unittest/optional/.  They were originally copied from libstdc++'s
testsuite, to make sure our gdb::optional behaved the same as std::optional,
in the subset of features it did support.  See d35d19584cf5.

BTW, with C++17, gdb::string_view can be replaced with std::string_view too,
and the whole of gdb/unittests/basic_string_view/ can go away too.

Pedro Alves

> 
> GDB 14 branched, so I think we're ready for that now.
> 
>>> +#if __cplusplus < 201703L
> 
> Andrew> Ignoring my previous comment for a moment; we now rely on this version
> Andrew> number being defined the same in two files.  Would it be better to move
> Andrew> the '#define GDB_OPTIONAL' into gdb_optional.h?  Then we can be certain
> Andrew> that it will be defined correctly, and in sync with any future changes
> Andrew> made in that file?
> 
> I don't think that would really help.  IIUC the issue would be compiling
> two source files with different -std settings.  But in this case
> gdb_optional.h would react to this.  Maybe I'm not understanding what
> you mean though.
> 
> It seems to me that we simply don't need to support compiling the tree
> with different std settings for different files -- it should be fine to
> just assume that everything is compiled compatibly.
> 
> Tom

Re: [PATCH] Use std::optional when available

[Andrew Burgess]

Tom Tromey <tromey@adacore.com> writes:

>>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
>
> Andrew> I think this change needs to be held off until we move to C++17 - at
> Andrew> which point gdb::optional can just be dropped.
>
> Once we switch I think we can just remove this code entirely.
>
> GDB 14 branched, so I think we're ready for that now.
>
>>> +#if __cplusplus < 201703L
>
> Andrew> Ignoring my previous comment for a moment; we now rely on this version
> Andrew> number being defined the same in two files.  Would it be better to move
> Andrew> the '#define GDB_OPTIONAL' into gdb_optional.h?  Then we can be certain
> Andrew> that it will be defined correctly, and in sync with any future changes
> Andrew> made in that file?
>
> I don't think that would really help.  IIUC the issue would be compiling
> two source files with different -std settings.  But in this case
> gdb_optional.h would react to this.  Maybe I'm not understanding what
> you mean though.

I think you agreed above that we can just move to C++17 and then we'll
just move to std::optional throughout, and, like Pedro says, all these
tests will go.

If I'm wrong, and you would like me to explain what I was driving at,
just let me know and I'll draft a patch to explain what I mean.

> It seems to me that we simply don't need to support compiling the tree
> with different std settings for different files -- it should be fine to
> just assume that everything is compiled compatibly.

Yeah, I would expect the result of using different std settings to be
"undefined", might work, but don't be surprised if it doesn't.

Thanks,
Andrew