* [PATCH] Use std::optional when available @ 2023-10-05 16:05 Tom Tromey 2023-10-09 12:40 ` Andrew Burgess 0 siblings, 1 reply; 5+ messages in thread From: Tom Tromey @ 2023-10-05 16:05 UTC (permalink / raw) To: gdb-patches; +Cc: 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Use std::optional when available 2023-10-05 16:05 [PATCH] Use std::optional when available Tom Tromey @ 2023-10-09 12:40 ` Andrew Burgess 2023-10-10 16:49 ` Tom Tromey 0 siblings, 1 reply; 5+ messages in thread From: Andrew Burgess @ 2023-10-09 12:40 UTC (permalink / raw) To: Tom Tromey via Gdb-patches, gdb-patches; +Cc: Tom Tromey 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Use std::optional when available 2023-10-09 12:40 ` Andrew Burgess @ 2023-10-10 16:49 ` Tom Tromey 2023-10-12 18:44 ` Pedro Alves 2023-10-16 14:13 ` Andrew Burgess 0 siblings, 2 replies; 5+ messages in thread From: Tom Tromey @ 2023-10-10 16:49 UTC (permalink / raw) To: Andrew Burgess; +Cc: Tom Tromey via Gdb-patches, 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Use std::optional when available 2023-10-10 16:49 ` Tom Tromey @ 2023-10-12 18:44 ` Pedro Alves 2023-10-16 14:13 ` Andrew Burgess 1 sibling, 0 replies; 5+ messages in thread From: Pedro Alves @ 2023-10-12 18:44 UTC (permalink / raw) To: Tom Tromey, Andrew Burgess; +Cc: Tom Tromey via Gdb-patches 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Use std::optional when available 2023-10-10 16:49 ` Tom Tromey 2023-10-12 18:44 ` Pedro Alves @ 2023-10-16 14:13 ` Andrew Burgess 1 sibling, 0 replies; 5+ messages in thread From: Andrew Burgess @ 2023-10-16 14:13 UTC (permalink / raw) To: Tom Tromey; +Cc: Tom Tromey via Gdb-patches, Tom Tromey 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-10-16 14:13 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-10-05 16:05 [PATCH] Use std::optional when available Tom Tromey 2023-10-09 12:40 ` Andrew Burgess 2023-10-10 16:49 ` Tom Tromey 2023-10-12 18:44 ` Pedro Alves 2023-10-16 14:13 ` Andrew Burgess
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).