public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 3/7] More gdb::optional features
  2017-04-10 12:22 [PATCH 0/7] C++fy xml-support.c and more gdb::optional Pedro Alves
  2017-04-10 12:22 ` [PATCH 5/7] xml-support.c: Use std::string for growing string buffer Pedro Alves
@ 2017-04-10 12:22 ` Pedro Alves
  2017-04-18 23:00   ` Pedro Alves
  2017-04-10 12:22 ` [PATCH 1/7] C++-ify gdb/xml-support.c a bit to eliminate cleanups Pedro Alves
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2017-04-10 12:22 UTC (permalink / raw)
  To: gdb-patches

The next feature I'm missing most from gdb::optional<T> compared to
std::optional<T> is construction/move/assignment from a T, instead of
having to default construct an gdb::optional and then use
optional::emplace(....).

For example:
  gdb::optional<std::string> function ()
  {
    gdb::optional<std::string> opt;
    std::string str;
    ...
    opt.emplace (std::move (str));
    return opt;
vs
  gdb::optional<std::string> function ()
  {
    std::string str;
    ...
    return str;

Those copy/move ctor/assign methods weren't initialy implemented
because std::optional supports construction from a type U if U is
convertible to T too, and has rules to decide whether the ctors are
explicit/implicit based on that, and rules for whether the ctor should
be trivial or not, etc., which leads to a much more complicated
implementation.

If we stick to supporting copy/move construction/assignment of/to an
optional<T> from exactly only optional<T> and T, then all that
conversion-related complication disappears, and we still gain
convenience in most use cases.

The patch also makes emplace return a reference to the constructor
object, per C++17 std::optional, and adds a reset method, againt
because std::optional has one and it's trivial to support it.  These
two changes are a requirement of the gdb::optional unit testing patch
that will follow.

(I'm making an effort to avoid importing the whole of libstdc++'s
std::optional.  If folks would be OK with that, it would certainly
work for me.  But I'd prefer doing it as a follow up.)

gdb/ChangeLog:
2017-04-07  Pedro Alves  <palves@redhat.com>

	* common/gdb_optional.h: Include common/traits.h.
	(in_place_t): New type.
	(in_place): New constexpr variable.
	(optional::optional): Remove member initialization of
	m_instantiated.
	(optional::optional(in_place_t...)): New constructor.
	(optional::~optional): Use reset.
	(optional::optional(const optional&)): New.
	(optional::optional(const optional&&)): New.
	(optional::optional(T &)): New.
	(optional::optional(T &&)): New.
	(operator::operator=(const optional &)): New.
	(operator::operator=(optional &&)): New.
	(operator::operator= (const T &))
	(operator::operator= (T &&))
	(operator::emplace (Args &&... args)): Return a T&.  Use reset.
	(operator::reset): New.
	(operator::m_instantiated):: Add in-class initializer.
	* common/traits.h: Include <type_traits>.
	(struct And): New types.
---
 gdb/common/gdb_optional.h | 132 ++++++++++++++++++++++++++++++++++++++++------
 gdb/common/traits.h       |  26 +++++++++
 2 files changed, 143 insertions(+), 15 deletions(-)

diff --git a/gdb/common/gdb_optional.h b/gdb/common/gdb_optional.h
index ad1119f..c30fa5b 100644
--- a/gdb/common/gdb_optional.h
+++ b/gdb/common/gdb_optional.h
@@ -20,46 +20,141 @@
 #ifndef GDB_OPTIONAL_H
 #define GDB_OPTIONAL_H
 
+#include "common/traits.h"
+
 namespace gdb
 {
 
+struct in_place_t
+{
+  explicit in_place_t () = default;
+};
+
+constexpr gdb::in_place_t in_place {};
+
 /* 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.  */
+   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.  */
+
 template<typename T>
 class optional
 {
 public:
 
   constexpr optional ()
-    : m_dummy (),
-      m_instantiated (false)
+    : m_dummy ()
+  {}
+
+  template<typename... Args>
+  constexpr optional (in_place_t, Args &&... args)
+    : m_item (std::forward<Args> (args)...),
+      m_instantiated (true)
+  {}
+
+  ~optional ()
+  { this->reset (); }
+
+  /* Copy and move constructors.  */
+
+  optional (const optional &other)
   {
+    if (other.m_instantiated)
+      this->emplace (other.get ());
   }
 
-  ~optional ()
+  optional (optional &&other)
+    noexcept(std::is_nothrow_move_constructible<T> ())
+  {
+    if (other.m_instantiated)
+      this->emplace (std::move (other.get ()));
+  }
+
+  constexpr optional (const T &other)
+    : m_item (other),
+      m_instantiated (true)
+  {}
+
+  constexpr optional (T &&other)
+    noexcept (std::is_nothrow_move_constructible<T> ())
+    : m_item (std::move (other)),
+      m_instantiated (true)
+  {}
+
+  /* Assignment operators.  */
+
+  optional &
+  operator= (const optional &other)
+  {
+    if (m_instantiated && other.m_instantiated)
+      this->get () = other.get ();
+    else
+      {
+	if (other.m_instantiated)
+	  this->emplace (other.get ());
+	else
+	  this->reset ();
+      }
+
+    return *this;
+  }
+
+  optional &
+  operator= (optional &&other)
+    noexcept (And<std::is_nothrow_move_constructible<T>,
+	      std::is_nothrow_move_assignable<T>> ())
+  {
+    if (m_instantiated && other.m_instantiated)
+      this->get () = std::move (other.get ());
+    else
+      {
+	if (other.m_instantiated)
+	  this->emplace (std::move (other.get ()));
+	else
+	  this->reset ();
+      }
+    return *this;
+  }
+
+  optional &
+  operator= (const T &other)
   {
     if (m_instantiated)
-      destroy ();
+      this->get () = other;
+    else
+      this->emplace (other);
+    return *this;
   }
 
-  /* These aren't deleted in std::optional, but it was simpler to
-     delete them here, because currently the users of this class don't
-     need them, and making them depend on the definition of T is
-     somewhat complicated.  */
-  optional (const optional &other) = delete;
-  optional<T> &operator= (const optional &other) = delete;
+  optional &
+  operator= (T &&other)
+    noexcept (And<std::is_nothrow_move_constructible<T>,
+	      std::is_nothrow_move_assignable<T>> ())
+  {
+    if (m_instantiated)
+      this->get () = std::move (other);
+    else
+      this->emplace (std::move (other));
+    return *this;
+  }
 
   template<typename... Args>
-  void emplace (Args &&... args)
+  T &emplace (Args &&... args)
   {
-    if (m_instantiated)
-      destroy ();
+    this->reset ();
     new (&m_item) T (std::forward<Args>(args)...);
     m_instantiated = true;
+    return this->get ();
   }
 
   /* Observers.  */
@@ -87,6 +182,13 @@ public:
   constexpr bool has_value () const noexcept
   { return m_instantiated; }
 
+  /* 'reset' is a 'safe' operation with no precondition.  */
+  void reset () noexcept
+  {
+    if (m_instantiated)
+      this->destroy ();
+  }
+
 private:
 
   /* Destroy the object.  */
@@ -109,7 +211,7 @@ private:
   };
 
   /* True if the object was ever emplaced.  */
-  bool m_instantiated;
+  bool m_instantiated = false;
 };
 
 }
diff --git a/gdb/common/traits.h b/gdb/common/traits.h
index 4b7bac3..c8f29cc 100644
--- a/gdb/common/traits.h
+++ b/gdb/common/traits.h
@@ -18,6 +18,8 @@
 #ifndef COMMON_TRAITS_H
 #define COMMON_TRAITS_H
 
+#include <type_traits>
+
 namespace gdb {
 
 /* Pre C++14-safe (CWG 1558) version of C++17's std::void_t.  See
@@ -29,6 +31,30 @@ struct make_void { typedef void type; };
 template<typename... Ts>
 using void_t = typename make_void<Ts...>::type;
 
+/* A few trait helpers, mainly stolen from libstdc++.  Uppercase
+   because "and" is a keyword.  */
+
+template<typename...>
+struct And;
+
+template<>
+struct And<> : public std::true_type
+{};
+
+template<typename B1>
+struct And<B1> : public B1
+{};
+
+template<typename B1, typename B2>
+struct And<B1, B2>
+  : public std::conditional<B1::value, B2, B1>::type
+{};
+
+template<typename B1, typename B2, typename B3, typename... Bn>
+struct And<B1, B2, B3, Bn...>
+  : public std::conditional<B1::value, And<B2, B3, Bn...>, B1>::type
+{};
+
 }
 
 #endif /* COMMON_TRAITS_H */
-- 
2.5.5

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 7/7] Eliminate obstack_printf
  2017-04-10 12:22 [PATCH 0/7] C++fy xml-support.c and more gdb::optional Pedro Alves
                   ` (2 preceding siblings ...)
  2017-04-10 12:22 ` [PATCH 1/7] C++-ify gdb/xml-support.c a bit to eliminate cleanups Pedro Alves
@ 2017-04-10 12:22 ` Pedro Alves
  2017-04-10 12:22 ` [PATCH 6/7] gdb_xml_parser: make data fields private and make more functions methods Pedro Alves
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2017-04-10 12:22 UTC (permalink / raw)
  To: gdb-patches

Not used anywhere.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* xml-support.c (obstack_xml_printf): Delete.
	* xml-support.h (obstack_xml_printf): Delete.
---
 gdb/xml-support.c | 40 ----------------------------------------
 gdb/xml-support.h |  7 -------
 2 files changed, 47 deletions(-)

diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index 9595246..6ca1630 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -997,46 +997,6 @@ show_debug_xml (struct ui_file *file, int from_tty,
   fprintf_filtered (file, _("XML debugging is %s.\n"), value);
 }
 
-void
-obstack_xml_printf (struct obstack *obstack, const char *format, ...)
-{
-  va_list ap;
-  const char *f;
-  const char *prev;
-  int percent = 0;
-
-  va_start (ap, format);
-
-  prev = format;
-  for (f = format; *f; f++)
-    {
-      if (percent)
-       {
-         switch (*f)
-           {
-           case 's':
-             {
-               char *p;
-               char *a = va_arg (ap, char *);
-
-               obstack_grow (obstack, prev, f - prev - 1);
-               p = xml_escape_text (a);
-               obstack_grow_str (obstack, p);
-               xfree (p);
-               prev = f + 1;
-             }
-             break;
-           }
-         percent = 0;
-       }
-      else if (*f == '%')
-       percent = 1;
-    }
-
-  obstack_grow_str (obstack, prev);
-  va_end (ap);
-}
-
 char *
 xml_fetch_content_from_file (const char *filename, void *baton)
 {
diff --git a/gdb/xml-support.h b/gdb/xml-support.h
index f9ea64d..1a1b7fd 100644
--- a/gdb/xml-support.h
+++ b/gdb/xml-support.h
@@ -227,13 +227,6 @@ extern gdb_xml_attribute_handler gdb_xml_parse_attr_enum;
 ULONGEST gdb_xml_parse_ulongest (struct gdb_xml_parser *parser,
 				 const char *value);
 
-/* Simple printf to obstack function.  Current implemented formatters:
-   %s - grow an xml escaped text in OBSTACK.  */
-
-extern void obstack_xml_printf (struct obstack *obstack,
-                               const char *format, ...)
-  ATTRIBUTE_PRINTF_2;
-
 /* Open FILENAME, read all its text into memory, close it, and return
    the text.  If something goes wrong, return NULL and warn.  */
 
-- 
2.5.5

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 0/7] C++fy xml-support.c and more gdb::optional
@ 2017-04-10 12:22 Pedro Alves
  2017-04-10 12:22 ` [PATCH 5/7] xml-support.c: Use std::string for growing string buffer Pedro Alves
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Pedro Alves @ 2017-04-10 12:22 UTC (permalink / raw)
  To: gdb-patches

I've been sitting on this series for a while now as part of the whole
"eliminate cleanups" effort, and since other folks are looking at
eliminating cleanups too, I thought I'd get this one over with to
avoid duplication.

In one of the patches (and also in [1] and in another larger series
I'm working on) I kept wanting to use gdb::optional, but finding
"opt.emplace(....)" a bit cumbersome compared to the more natural
ctor/op=.  So I bit the bullet and brought in a bit more of C++17's
std::optional API into gdb::optional.

I then tried to follow the "Unit tests or it didn't happen (TM)"
principle and added unit tests for gdb::optional.  That payed off and
caught some bugs in the new code I was adding.

[1] - https://sourceware.org/ml/gdb-patches/2017-03/msg00497.html

Tested on x86-64 Fedora 23, native and gdbserver.

Pedro Alves (7):
  C++-ify gdb/xml-support.c a bit to eliminate cleanups
  xml-support.c: Use std::vector
  More gdb::optional features
  gdb::optional unit tests
  xml-support.c: Use std::string for growing string buffer
  gdb_xml_parser: make data fields private and make more functions
    methods
  Eliminate obstack_printf

 gdb/Makefile.in                        |   6 +-
 gdb/common/gdb_optional.h              | 132 +++++++-
 gdb/common/traits.h                    |  26 ++
 gdb/tracefile-tfile.c                  |  14 +-
 gdb/unittests/optional-selftests.c     |  94 ++++++
 gdb/unittests/optional/assignment/1.cc | 195 +++++++++++
 gdb/unittests/optional/assignment/2.cc | 193 +++++++++++
 gdb/unittests/optional/assignment/3.cc | 156 +++++++++
 gdb/unittests/optional/assignment/4.cc | 156 +++++++++
 gdb/unittests/optional/assignment/5.cc |  80 +++++
 gdb/unittests/optional/assignment/6.cc |  90 +++++
 gdb/unittests/optional/assignment/7.cc |  29 ++
 gdb/unittests/optional/cons/copy.cc    | 126 +++++++
 gdb/unittests/optional/cons/default.cc |  58 ++++
 gdb/unittests/optional/cons/move.cc    | 124 +++++++
 gdb/unittests/optional/cons/value.cc   | 294 +++++++++++++++++
 gdb/unittests/optional/in_place.cc     |  65 ++++
 gdb/unittests/optional/observers/1.cc  |  31 ++
 gdb/unittests/optional/observers/2.cc  |  35 ++
 gdb/xml-support.c                      | 577 ++++++++++++++++-----------------
 gdb/xml-support.h                      |  36 +-
 gdb/xml-tdesc.c                        |  81 ++---
 gdb/xml-tdesc.h                        |  17 +-
 23 files changed, 2206 insertions(+), 409 deletions(-)
 create mode 100644 gdb/unittests/optional-selftests.c
 create mode 100644 gdb/unittests/optional/assignment/1.cc
 create mode 100644 gdb/unittests/optional/assignment/2.cc
 create mode 100644 gdb/unittests/optional/assignment/3.cc
 create mode 100644 gdb/unittests/optional/assignment/4.cc
 create mode 100644 gdb/unittests/optional/assignment/5.cc
 create mode 100644 gdb/unittests/optional/assignment/6.cc
 create mode 100644 gdb/unittests/optional/assignment/7.cc
 create mode 100644 gdb/unittests/optional/cons/copy.cc
 create mode 100644 gdb/unittests/optional/cons/default.cc
 create mode 100644 gdb/unittests/optional/cons/move.cc
 create mode 100644 gdb/unittests/optional/cons/value.cc
 create mode 100644 gdb/unittests/optional/in_place.cc
 create mode 100644 gdb/unittests/optional/observers/1.cc
 create mode 100644 gdb/unittests/optional/observers/2.cc

-- 
2.5.5

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 5/7] xml-support.c: Use std::string for growing string buffer
  2017-04-10 12:22 [PATCH 0/7] C++fy xml-support.c and more gdb::optional Pedro Alves
@ 2017-04-10 12:22 ` Pedro Alves
  2017-04-15  4:36   ` Simon Marchi
  2017-04-10 12:22 ` [PATCH 3/7] More gdb::optional features Pedro Alves
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Pedro Alves @ 2017-04-10 12:22 UTC (permalink / raw)
  To: gdb-patches

This main idea behind this patch is this change to xml-support.c:scope_level

  -  /* Body text accumulation.  This is an owning pointer.  */
  -  struct obstack *body;
  +  /* Body text accumulation.  */
  +  std::string body;

... which allows simplifying other parts of the code.

In target_fetch_description_xml, we want to distinguish between
returning "success + empty std::string" and "no success", and
gdb::optional is a natural fit for that.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* tracefile-tfile.c (tfile_write_tdesc): Adjust to use
	gdb::optional<std::string>.
	* xml-support.c: Include <string>.
	(scope_level::scope_level(scope_level &&))
	(scope_level::~scope_level): Delete.
	(scope_level::body): Now a std::string.
	(gdb_xml_body_text, gdb_xml_end_element): Adjust.
	(xinclude_parsing_data::xinclude_parsing_data): Add 'output'
	parameter.
	(xinclude_parsing_data::~xinclude_parsing_data): Delete.
	(xinclude_parsing_data::output): Now a std::string reference.
	(xinclude_start_include): Adjust.
	(xml_xinclude_default): Adjust.
	(xml_process_xincludes): Add 'output' parameter, and return bool.
	* xml-support.h (xml_process_xincludes): Add 'output' parameter,
	and return bool.
	* xml-tdesc.c: Include <unordered_map> and <string>.
	(tdesc_xml_cache): Delete.
	(tdesc_xml_cache_s): Delete.
	(xml_cache): Now an std::unordered_map.
	(tdesc_parse_xml): Adjust to use std::string and unordered_map.
	(target_fetch_description_xml): Change return type to
	gdb::optional<std::string>, and adjust.
	* xml-tdesc.h: Include "common/gdb_optional.h" and <string>.
	(target_fetch_description_xml): Change return type to
	gdb::optional<std::string>.
---
 gdb/tracefile-tfile.c |  14 +++----
 gdb/xml-support.c     | 104 +++++++++++++++-----------------------------------
 gdb/xml-support.h     |  15 ++++----
 gdb/xml-tdesc.c       |  81 ++++++++++++++-------------------------
 gdb/xml-tdesc.h       |  17 +++++++--
 5 files changed, 88 insertions(+), 143 deletions(-)

diff --git a/gdb/tracefile-tfile.c b/gdb/tracefile-tfile.c
index 5d63c16..255bbc9 100644
--- a/gdb/tracefile-tfile.c
+++ b/gdb/tracefile-tfile.c
@@ -275,17 +275,19 @@ tfile_write_tdesc (struct trace_file_writer *self)
 {
   struct tfile_trace_file_writer *writer
     = (struct tfile_trace_file_writer *) self;
-  char *tdesc = target_fetch_description_xml (&current_target);
-  char *ptr = tdesc;
-  char *next;
 
-  if (tdesc == NULL)
+  gdb::optional<std::string> tdesc
+    = target_fetch_description_xml (&current_target);
+
+  if (!tdesc)
     return;
 
+  const char *ptr = tdesc->c_str ();
+
   /* Write tdesc line by line, prefixing each line with "tdesc ".  */
   while (ptr != NULL)
     {
-      next = strchr (ptr, '\n');
+      const char *next = strchr (ptr, '\n');
       if (next != NULL)
 	{
 	  fprintf (writer->fp, "tdesc %.*s\n", (int) (next - ptr), ptr);
@@ -299,8 +301,6 @@ tfile_write_tdesc (struct trace_file_writer *self)
 	}
       ptr = next;
     }
-
-  xfree (tdesc);
 }
 
 /* This is the implementation of trace_file_write_ops method
diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index b19f81a..91e31d9 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -23,6 +23,7 @@
 #include "filestuff.h"
 #include "safe-ctype.h"
 #include <vector>
+#include <string>
 
 /* Debugging flag.  */
 static int debug_xml;
@@ -46,29 +47,9 @@ struct scope_level
   explicit scope_level (const gdb_xml_element *elements_ = NULL)
     : elements (elements_),
       element (NULL),
-      seen (0),
-      body (NULL)
+      seen (0)
   {}
 
-  scope_level (scope_level &&other) noexcept
-    : elements (other.elements),
-      element (other.element),
-      seen (other.seen),
-      body (other.body)
-  {
-    if (this != &other)
-      other.body = NULL;
-  }
-
-  ~scope_level ()
-  {
-    if (this->body)
-      {
-	obstack_free (this->body, NULL);
-	xfree (this->body);
-      }
-  }
-
   /* Elements we allow at this level.  */
   const struct gdb_xml_element *elements;
 
@@ -79,8 +60,8 @@ struct scope_level
      optional and repeatable checking).  */
   unsigned int seen;
 
-  /* Body text accumulation.  This is an owning pointer.  */
-  struct obstack *body;
+  /* Body text accumulation.  */
+  std::string body;
 };
 
 /* The parser itself, and our additional state.  */
@@ -120,14 +101,7 @@ gdb_xml_body_text (void *data, const XML_Char *text, int length)
     return;
 
   scope_level &scope = parser->scopes.back ();
-
-  if (scope.body == NULL)
-    {
-      scope.body = XCNEW (struct obstack);
-      obstack_init (scope.body);
-    }
-
-  obstack_grow (scope.body, text, length);
+  scope.body.append (text, length);
 }
 
 /* Issue a debugging message from one of PARSER's handlers.  */
@@ -390,29 +364,27 @@ gdb_xml_end_element (gdb_xml_parser *parser, const XML_Char *name)
   /* Call the element processor.  */
   if (scope->element != NULL && scope->element->end_handler)
     {
-      const char *scope_body;
+      const char *body;
 
-      if (scope->body == NULL)
-	scope_body = "";
+      if (scope->body.empty ())
+	body = "";
       else
 	{
 	  int length;
 
-	  length = obstack_object_size (scope->body);
-	  obstack_1grow (scope->body, '\0');
-	  char *body = (char *) obstack_finish (scope->body);
+	  length = scope->body.size ();
+	  body = scope->body.c_str ();
 
 	  /* Strip leading and trailing whitespace.  */
-	  while (length > 0 && ISSPACE (body[length-1]))
-	    body[--length] = '\0';
+	  while (length > 0 && ISSPACE (body[length - 1]))
+	    length--;
+	  scope->body.erase (length);
 	  while (*body && ISSPACE (*body))
 	    body++;
-
-	  scope_body = body;
 	}
 
       scope->element->end_handler (parser, scope->element, parser->user_data,
-				   scope_body);
+				   body);
     }
   else if (scope->element == NULL)
     XML_DefaultCurrent (parser->expat_parser);
@@ -726,23 +698,18 @@ gdb_xml_parse_attr_enum (struct gdb_xml_parser *parser,
 
 struct xinclude_parsing_data
 {
-  xinclude_parsing_data (xml_fetch_another fetcher_, void *fetcher_baton_,
+  xinclude_parsing_data (std::string &output_,
+			 xml_fetch_another fetcher_, void *fetcher_baton_,
 			 int include_depth_)
-    : skip_depth (0),
+    : output (output_),
+      skip_depth (0),
       include_depth (include_depth_),
       fetcher (fetcher_),
       fetcher_baton (fetcher_baton_)
-  {
-    obstack_init (&this->obstack);
-  }
-
-  ~xinclude_parsing_data ()
-  {
-    obstack_free (&this->obstack, NULL);
-  }
+  {}
 
-  /* The obstack to build the output in.  */
-  struct obstack obstack;
+  /* Where the output goes.  */
+  std::string &output;
 
   /* A count indicating whether we are in an element whose
      children should not be copied to the output, and if so,
@@ -782,15 +749,11 @@ xinclude_start_include (struct gdb_xml_parser *parser,
     gdb_xml_error (parser, _("Could not load XML document \"%s\""), href);
   back_to = make_cleanup (xfree, text);
 
-  output = xml_process_xincludes (parser->name, text, data->fetcher,
-				  data->fetcher_baton,
-				  data->include_depth + 1);
-  if (output == NULL)
+  if (!xml_process_xincludes (data->output, parser->name, text, data->fetcher,
+			      data->fetcher_baton,
+			      data->include_depth + 1))
     gdb_xml_error (parser, _("Parsing \"%s\" failed"), href);
 
-  obstack_grow (&data->obstack, output, strlen (output));
-  xfree (output);
-
   do_cleanups (back_to);
 
   data->skip_depth++;
@@ -821,7 +784,7 @@ xml_xinclude_default (void *data_, const XML_Char *s, int len)
 
   /* Otherwise just add it to the end of the document we're building
      up.  */
-  obstack_grow (&data->obstack, s, len);
+  data->output.append (s, len);
 }
 
 static void XMLCALL
@@ -871,14 +834,13 @@ const struct gdb_xml_element xinclude_elements[] = {
 
 /* The main entry point for <xi:include> processing.  */
 
-char *
-xml_process_xincludes (const char *name, const char *text,
+bool
+xml_process_xincludes (std::string &result,
+		       const char *name, const char *text,
 		       xml_fetch_another fetcher, void *fetcher_baton,
 		       int depth)
 {
-  char *result = NULL;
-
-  xinclude_parsing_data data (fetcher, fetcher_baton, depth);
+  xinclude_parsing_data data (result, fetcher, fetcher_baton, depth);
 
   gdb_xml_parser parser (name, xinclude_elements, &data);
   parser.is_xinclude = true;
@@ -901,16 +863,12 @@ xml_process_xincludes (const char *name, const char *text,
 
   if (gdb_xml_parse (&parser, text) == 0)
     {
-      obstack_1grow (&data.obstack, '\0');
-      result = xstrdup ((const char *) obstack_finish (&data.obstack));
-
       if (depth == 0)
 	gdb_xml_debug (&parser, _("XInclude processing succeeded."));
+      return true;
     }
-  else
-    result = NULL;
 
-  return result;
+  return false;
 }
 #endif /* HAVE_LIBEXPAT */
 \f
diff --git a/gdb/xml-support.h b/gdb/xml-support.h
index de685e2..8248a32 100644
--- a/gdb/xml-support.h
+++ b/gdb/xml-support.h
@@ -54,17 +54,18 @@ extern const char *xml_builtin[][2];
 
 typedef char *(*xml_fetch_another) (const char *href, void *baton);
 
-/* Return a new string which is the expansion of TEXT after processing
-   <xi:include> tags.  FETCHER will be called (with FETCHER_BATON) to
-   retrieve any new files.  DEPTH should be zero on the initial call.
+/* Append the expansion of TEXT after processing <xi:include> tags in
+   RESULT.  FETCHER will be called (with FETCHER_BATON) to retrieve
+   any new files.  DEPTH should be zero on the initial call.
 
-   On failure, this function uses NAME in a warning and returns NULL.
+   On failure, this function uses NAME in a warning and returns false.
    It may throw an exception, but does not for XML parsing
    problems.  */
 
-char *xml_process_xincludes (const char *name, const char *text,
-			     xml_fetch_another fetcher, void *fetcher_baton,
-			     int depth);
+bool xml_process_xincludes (std::string &result,
+			    const char *name, const char *text,
+			    xml_fetch_another fetcher, void *fetcher_baton,
+			    int depth);
 
 /* Simplified XML parser infrastructure.  */
 
diff --git a/gdb/xml-tdesc.c b/gdb/xml-tdesc.c
index 522a9ba..daa713f 100644
--- a/gdb/xml-tdesc.c
+++ b/gdb/xml-tdesc.c
@@ -26,6 +26,8 @@
 #include "xml-tdesc.h"
 #include "osabi.h"
 #include "filenames.h"
+#include <unordered_map>
+#include <string>
 
 /* Maximum sizes.
    This is just to catch obviously wrong values.  */
@@ -64,15 +66,7 @@ tdesc_parse_xml (const char *document, xml_fetch_another fetcher,
    then we will create unnecessary duplicate gdbarches.  See
    gdbarch_list_lookup_by_info.  */
 
-struct tdesc_xml_cache
-{
-  const char *xml_document;
-  struct target_desc *tdesc;
-};
-typedef struct tdesc_xml_cache tdesc_xml_cache_s;
-DEF_VEC_O(tdesc_xml_cache_s);
-
-static VEC(tdesc_xml_cache_s) *xml_cache;
+static std::unordered_map<std::string, target_desc *> xml_cache;
 
 /* Callback data for target description parsing.  */
 
@@ -631,55 +625,42 @@ static struct target_desc *
 tdesc_parse_xml (const char *document, xml_fetch_another fetcher,
 		 void *fetcher_baton)
 {
-  struct cleanup *back_to, *result_cleanup;
   struct tdesc_parsing_data data;
-  struct tdesc_xml_cache *cache;
-  char *expanded_text;
-  int ix;
 
   /* Expand all XInclude directives.  */
-  expanded_text = xml_process_xincludes (_("target description"),
-					 document, fetcher, fetcher_baton, 0);
-  if (expanded_text == NULL)
+  std::string expanded_text;
+
+  if (!xml_process_xincludes (expanded_text,
+			      _("target description"),
+			      document, fetcher, fetcher_baton, 0))
     {
       warning (_("Could not load XML target description; ignoring"));
       return NULL;
     }
 
   /* Check for an exact match in the list of descriptions we have
-     previously parsed.  strcmp is a slightly inefficient way to
-     do this; an SHA-1 checksum would work as well.  */
-  for (ix = 0; VEC_iterate (tdesc_xml_cache_s, xml_cache, ix, cache); ix++)
-    if (strcmp (cache->xml_document, expanded_text) == 0)
-      {
-       xfree (expanded_text);
-       return cache->tdesc;
-      }
-
-  back_to = make_cleanup (null_cleanup, NULL);
+     previously parsed.  */
+  const auto it = xml_cache.find (expanded_text);
+  if (it != xml_cache.end ())
+    return it->second;
 
   memset (&data, 0, sizeof (struct tdesc_parsing_data));
   data.tdesc = allocate_target_description ();
-  result_cleanup = make_cleanup_free_target_description (data.tdesc);
-  make_cleanup (xfree, expanded_text);
+  struct cleanup *result_cleanup
+    = make_cleanup_free_target_description (data.tdesc);
 
   if (gdb_xml_parse_quick (_("target description"), "gdb-target.dtd",
-			   tdesc_elements, expanded_text, &data) == 0)
+			   tdesc_elements, expanded_text.c_str (), &data) == 0)
     {
       /* Parsed successfully.  */
-      struct tdesc_xml_cache new_cache;
-
-      new_cache.xml_document = expanded_text;
-      new_cache.tdesc = data.tdesc;
-      VEC_safe_push (tdesc_xml_cache_s, xml_cache, &new_cache);
+      xml_cache.emplace (std::move (expanded_text), data.tdesc);
       discard_cleanups (result_cleanup);
-      do_cleanups (back_to);
       return data.tdesc;
     }
   else
     {
       warning (_("Could not load XML target description; ignoring"));
-      do_cleanups (back_to);
+      do_cleanups (result_cleanup);
       return NULL;
     }
 }
@@ -759,7 +740,7 @@ target_read_description_xml (struct target_ops *ops)
    includes, but not parsing it.  Used to dump whole tdesc
    as a single XML file.  */
 
-char *
+gdb::optional<std::string>
 target_fetch_description_xml (struct target_ops *ops)
 {
 #if !defined(HAVE_LIBEXPAT)
@@ -772,28 +753,24 @@ target_fetch_description_xml (struct target_ops *ops)
 		 "disabled at compile time"));
     }
 
-  return NULL;
+  return {};
 #else
   struct target_desc *tdesc;
-  char *tdesc_str;
-  char *expanded_text;
-  struct cleanup *back_to;
 
-  tdesc_str = fetch_available_features_from_target ("target.xml", ops);
+  gdb::unique_xmalloc_ptr<char>
+    tdesc_str (fetch_available_features_from_target ("target.xml", ops));
   if (tdesc_str == NULL)
-    return NULL;
+    return {};
 
-  back_to = make_cleanup (xfree, tdesc_str);
-  expanded_text = xml_process_xincludes (_("target description"),
-					 tdesc_str,
-					 fetch_available_features_from_target, ops, 0);
-  do_cleanups (back_to);
-  if (expanded_text == NULL)
+  std::string output;
+  if (!xml_process_xincludes (output,
+			      _("target description"),
+			      tdesc_str.get (),
+			      fetch_available_features_from_target, ops, 0))
     {
       warning (_("Could not load XML target description; ignoring"));
-      return NULL;
+      return {};
     }
-
-  return expanded_text;
+  return output;
 #endif
 }
diff --git a/gdb/xml-tdesc.h b/gdb/xml-tdesc.h
index 1637a89..2375395 100644
--- a/gdb/xml-tdesc.h
+++ b/gdb/xml-tdesc.h
@@ -19,6 +19,12 @@
    You should have received a copy of the GNU General Public License
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
+#ifndef XML_TDESC_H
+#define XML_TDESC_H
+
+#include "common/gdb_optional.h"
+#include <string>
+
 struct target_ops;
 struct target_desc;
 
@@ -32,8 +38,11 @@ const struct target_desc *file_read_description_xml (const char *filename);
 
 const struct target_desc *target_read_description_xml (struct target_ops *);
 
-/* Fetches an XML target description using OPS,  processing
-   includes, but not parsing it.  Used to dump whole tdesc
-   as a single XML file.  */
+/* Fetches an XML target description using OPS, processing includes,
+   but not parsing it.  Used to dump whole tdesc as a single XML file.
+   Returns the description on success, and a disengaged optional
+   otherwise.  */
+gdb::optional<std::string> target_fetch_description_xml (target_ops *ops);
+
+#endif /* XML_TDESC_H */
 
-char *target_fetch_description_xml (struct target_ops *ops);
-- 
2.5.5

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/7] C++-ify gdb/xml-support.c a bit to eliminate cleanups
  2017-04-10 12:22 [PATCH 0/7] C++fy xml-support.c and more gdb::optional Pedro Alves
  2017-04-10 12:22 ` [PATCH 5/7] xml-support.c: Use std::string for growing string buffer Pedro Alves
  2017-04-10 12:22 ` [PATCH 3/7] More gdb::optional features Pedro Alves
@ 2017-04-10 12:22 ` Pedro Alves
  2017-04-10 12:22 ` [PATCH 7/7] Eliminate obstack_printf Pedro Alves
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2017-04-10 12:22 UTC (permalink / raw)
  To: gdb-patches

Basically convert cleanups to destructors in gdb_xml_parser and
xinclude_parsing_data, and then allocate objects of those types on the
stack.

More C++-ification is possible / will follow, but this removes a few
make_cleanup calls already.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* xml-support.c (gdb_xml_parser): Add ctor/dtor.  Make is_xinclude
	a bool.
	(gdb_xml_end_element): Change type of first parameter.
	(gdb_xml_cleanup): Rename to ...
	(gdb_xml_parser::~gdb_xml_parser): ... this.
	(gdb_xml_create_parser_and_cleanup): Delete with ...
	(gdb_xml_parser::gdb_xml_parser): ... creation parts factored out
	to this new ctor.
	(gdb_xml_parse_quick): Create a local gdb_xml_parser instead of
	using gdb_xml_create_parser_and_cleanup.
	(xinclude_parsing_data): Add ctor/dtor.
	(xml_xinclude_cleanup): Delete.
	(xml_process_xincludes): Create a local xinclude_parsing_data
	instead of heap-allocating one.  Create a local gdb_xml_parser
	instead of heap-allocating one with
	gdb_xml_create_parser_and_cleanup.
---
 gdb/xml-support.c | 150 +++++++++++++++++++++++-------------------------------
 1 file changed, 63 insertions(+), 87 deletions(-)

diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index ea0657d..d9dc735 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -61,6 +61,11 @@ DEF_VEC_O(scope_level_s);
 /* The parser itself, and our additional state.  */
 struct gdb_xml_parser
 {
+  gdb_xml_parser (const char *name,
+		  const struct gdb_xml_element *elements,
+		  void *user_data);
+  ~gdb_xml_parser();
+
   XML_Parser expat_parser;	/* The underlying expat parser.  */
 
   const char *name;		/* Name of this parser.  */
@@ -73,7 +78,7 @@ struct gdb_xml_parser
 
   const char *dtd_name;		/* The name of the expected / default DTD,
 				   if specified.  */
-  int is_xinclude;		/* Are we the special <xi:include> parser?  */
+  bool is_xinclude;		/* Are we the special <xi:include> parser?  */
 };
 
 /* Process some body text.  We accumulate the text for later use; it's
@@ -331,13 +336,12 @@ gdb_xml_start_element_wrapper (void *data, const XML_Char *name,
   END_CATCH
 }
 
-/* Handle the end of an element.  DATA is our local XML parser, and
+/* Handle the end of an element.  PARSER is our local XML parser, and
    NAME is the current element.  */
 
 static void
-gdb_xml_end_element (void *data, const XML_Char *name)
+gdb_xml_end_element (gdb_xml_parser *parser, const XML_Char *name)
 {
-  struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data;
   struct scope_level *scope = VEC_last (scope_level_s, parser->scopes);
   const struct gdb_xml_element *element;
   unsigned int seen;
@@ -404,7 +408,7 @@ gdb_xml_end_element_wrapper (void *data, const XML_Char *name)
 
   TRY
     {
-      gdb_xml_end_element (data, name);
+      gdb_xml_end_element (parser, name);
     }
   CATCH (ex, RETURN_MASK_ALL)
     {
@@ -418,66 +422,52 @@ gdb_xml_end_element_wrapper (void *data, const XML_Char *name)
 
 /* Free a parser and all its associated state.  */
 
-static void
-gdb_xml_cleanup (void *arg)
+gdb_xml_parser::~gdb_xml_parser ()
 {
-  struct gdb_xml_parser *parser = (struct gdb_xml_parser *) arg;
   struct scope_level *scope;
   int ix;
 
-  XML_ParserFree (parser->expat_parser);
+  XML_ParserFree (this->expat_parser);
 
   /* Clean up the scopes.  */
-  for (ix = 0; VEC_iterate (scope_level_s, parser->scopes, ix, scope); ix++)
+  for (ix = 0; VEC_iterate (scope_level_s, this->scopes, ix, scope); ix++)
     if (scope->body)
       {
 	obstack_free (scope->body, NULL);
 	xfree (scope->body);
       }
-  VEC_free (scope_level_s, parser->scopes);
-
-  xfree (parser);
+  VEC_free (scope_level_s, this->scopes);
 }
 
-/* Initialize a parser and store it to *PARSER_RESULT.  Register a
-   cleanup to destroy the parser.  */
-
-static struct cleanup *
-gdb_xml_create_parser_and_cleanup (const char *name,
-				   const struct gdb_xml_element *elements,
-				   void *user_data,
-				   struct gdb_xml_parser **parser_result)
+/* Initialize a parser.  */
+
+gdb_xml_parser::gdb_xml_parser (const char *name_,
+				const gdb_xml_element *elements,
+				void *user_data_)
+  : name (name_),
+    user_data (user_data_),
+    scopes (NULL),
+    error (exception_none),
+    last_line (0),
+    dtd_name (NULL),
+    is_xinclude (false)
 {
-  struct gdb_xml_parser *parser;
-  struct scope_level start_scope;
-  struct cleanup *result;
-
-  /* Initialize the parser.  */
-  parser = XCNEW (struct gdb_xml_parser);
-  parser->expat_parser = XML_ParserCreateNS (NULL, '!');
-  if (parser->expat_parser == NULL)
-    {
-      xfree (parser);
-      malloc_failure (0);
-    }
-
-  parser->name = name;
+  this->expat_parser = XML_ParserCreateNS (NULL, '!');
+  if (this->expat_parser == NULL)
+    malloc_failure (0);
 
-  parser->user_data = user_data;
-  XML_SetUserData (parser->expat_parser, parser);
+  XML_SetUserData (this->expat_parser, this);
 
   /* Set the callbacks.  */
-  XML_SetElementHandler (parser->expat_parser, gdb_xml_start_element_wrapper,
+  XML_SetElementHandler (this->expat_parser, gdb_xml_start_element_wrapper,
 			 gdb_xml_end_element_wrapper);
-  XML_SetCharacterDataHandler (parser->expat_parser, gdb_xml_body_text);
+  XML_SetCharacterDataHandler (this->expat_parser, gdb_xml_body_text);
 
   /* Initialize the outer scope.  */
+  scope_level start_scope;
   memset (&start_scope, 0, sizeof (start_scope));
   start_scope.elements = elements;
-  VEC_safe_push (scope_level_s, parser->scopes, &start_scope);
-
-  *parser_result = parser;
-  return make_cleanup (gdb_xml_cleanup, parser);
+  VEC_safe_push (scope_level_s, this->scopes, &start_scope);
 }
 
 /* External entity handler.  The only external entities we support
@@ -603,19 +593,10 @@ gdb_xml_parse_quick (const char *name, const char *dtd_name,
 		     const struct gdb_xml_element *elements,
 		     const char *document, void *user_data)
 {
-  struct gdb_xml_parser *parser;
-  struct cleanup *back_to;
-  int result;
-
-  back_to = gdb_xml_create_parser_and_cleanup (name, elements,
-					       user_data, &parser);
+  gdb_xml_parser parser (name, elements, user_data);
   if (dtd_name != NULL)
-    gdb_xml_use_dtd (parser, dtd_name);
-  result = gdb_xml_parse (parser, document);
-
-  do_cleanups (back_to);
-
-  return result;
+    gdb_xml_use_dtd (&parser, dtd_name);
+  return gdb_xml_parse (&parser, document);
 }
 
 /* Parse a field VALSTR that we expect to contain an integer value.
@@ -735,6 +716,21 @@ gdb_xml_parse_attr_enum (struct gdb_xml_parser *parser,
 
 struct xinclude_parsing_data
 {
+  xinclude_parsing_data (xml_fetch_another fetcher_, void *fetcher_baton_,
+			 int include_depth_)
+    : skip_depth (0),
+      include_depth (include_depth_),
+      fetcher (fetcher_),
+      fetcher_baton (fetcher_baton_)
+  {
+    obstack_init (&this->obstack);
+  }
+
+  ~xinclude_parsing_data ()
+  {
+    obstack_free (&this->obstack, NULL);
+  }
+
   /* The obstack to build the output in.  */
   struct obstack obstack;
 
@@ -851,15 +847,6 @@ xml_xinclude_xml_decl (void *data_, const XML_Char *version,
      output.  */
 }
 
-static void
-xml_xinclude_cleanup (void *data_)
-{
-  struct xinclude_parsing_data *data = (struct xinclude_parsing_data *) data_;
-
-  obstack_free (&data->obstack, NULL);
-  xfree (data);
-}
-
 const struct gdb_xml_attribute xinclude_attributes[] = {
   { "href", GDB_XML_AF_NONE, NULL, NULL },
   { NULL, GDB_XML_AF_NONE, NULL, NULL }
@@ -879,51 +866,40 @@ xml_process_xincludes (const char *name, const char *text,
 		       xml_fetch_another fetcher, void *fetcher_baton,
 		       int depth)
 {
-  struct gdb_xml_parser *parser;
-  struct xinclude_parsing_data *data;
-  struct cleanup *back_to;
   char *result = NULL;
 
-  data = XCNEW (struct xinclude_parsing_data);
-  obstack_init (&data->obstack);
-  back_to = make_cleanup (xml_xinclude_cleanup, data);
-
-  gdb_xml_create_parser_and_cleanup (name, xinclude_elements,
-				     data, &parser);
-  parser->is_xinclude = 1;
+  xinclude_parsing_data data (fetcher, fetcher_baton, depth);
 
-  data->include_depth = depth;
-  data->fetcher = fetcher;
-  data->fetcher_baton = fetcher_baton;
+  gdb_xml_parser parser (name, xinclude_elements, &data);
+  parser.is_xinclude = true;
 
-  XML_SetCharacterDataHandler (parser->expat_parser, NULL);
-  XML_SetDefaultHandler (parser->expat_parser, xml_xinclude_default);
+  XML_SetCharacterDataHandler (parser.expat_parser, NULL);
+  XML_SetDefaultHandler (parser.expat_parser, xml_xinclude_default);
 
   /* Always discard the XML version declarations; the only important
      thing this provides is encoding, and our result will have been
      converted to UTF-8.  */
-  XML_SetXmlDeclHandler (parser->expat_parser, xml_xinclude_xml_decl);
+  XML_SetXmlDeclHandler (parser.expat_parser, xml_xinclude_xml_decl);
 
   if (depth > 0)
     /* Discard the doctype for included documents.  */
-    XML_SetDoctypeDeclHandler (parser->expat_parser,
+    XML_SetDoctypeDeclHandler (parser.expat_parser,
 			       xml_xinclude_start_doctype,
 			       xml_xinclude_end_doctype);
 
-  gdb_xml_use_dtd (parser, "xinclude.dtd");
+  gdb_xml_use_dtd (&parser, "xinclude.dtd");
 
-  if (gdb_xml_parse (parser, text) == 0)
+  if (gdb_xml_parse (&parser, text) == 0)
     {
-      obstack_1grow (&data->obstack, '\0');
-      result = xstrdup ((const char *) obstack_finish (&data->obstack));
+      obstack_1grow (&data.obstack, '\0');
+      result = xstrdup ((const char *) obstack_finish (&data.obstack));
 
       if (depth == 0)
-	gdb_xml_debug (parser, _("XInclude processing succeeded."));
+	gdb_xml_debug (&parser, _("XInclude processing succeeded."));
     }
   else
     result = NULL;
 
-  do_cleanups (back_to);
   return result;
 }
 #endif /* HAVE_LIBEXPAT */
-- 
2.5.5

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/7] xml-support.c: Use std::vector
  2017-04-10 12:22 [PATCH 0/7] C++fy xml-support.c and more gdb::optional Pedro Alves
                   ` (4 preceding siblings ...)
  2017-04-10 12:22 ` [PATCH 6/7] gdb_xml_parser: make data fields private and make more functions methods Pedro Alves
@ 2017-04-10 12:22 ` Pedro Alves
  2017-04-10 12:32 ` [PATCH 4/7] gdb::optional unit tests Pedro Alves
  2017-04-15 16:20 ` [PATCH 0/7] C++fy xml-support.c and more gdb::optional Simon Marchi
  7 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2017-04-10 12:22 UTC (permalink / raw)
  To: gdb-patches

scope_level::scope_level needed both a move ctor and a dtor explicitly
coded, but those will be eliminated in a following patch.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* xml-support.c: Include <vector>.
	(scope_level::scope_level(const gdb_xml_element *))
	(scope_level::scope_level(scope_level&&)): New.
	(scope_level::~scope_level): New.
	(scope_level_s): Delete.
	(gdb_xml_parser::scopes): Now a std::vector.
	(gdb_xml_body_text, gdb_xml_start_element, gdb_xml_end_element):
	Use std::vector.
	(gdb_xml_parser::~gdb_xml_parser): Remove now unnecessary
	scope cleanup code.
	(gdb_xml_parser::gdb_xml_parser): Remove explicit initialization
	of the scopes member.  Use std::vector::emplace_back.
---
 gdb/xml-support.c | 112 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 51 deletions(-)

diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index d9dc735..b19f81a 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -22,6 +22,7 @@
 #include "xml-support.h"
 #include "filestuff.h"
 #include "safe-ctype.h"
+#include <vector>
 
 /* Debugging flag.  */
 static int debug_xml;
@@ -42,6 +43,32 @@ static int debug_xml;
    nesting.  */
 struct scope_level
 {
+  explicit scope_level (const gdb_xml_element *elements_ = NULL)
+    : elements (elements_),
+      element (NULL),
+      seen (0),
+      body (NULL)
+  {}
+
+  scope_level (scope_level &&other) noexcept
+    : elements (other.elements),
+      element (other.element),
+      seen (other.seen),
+      body (other.body)
+  {
+    if (this != &other)
+      other.body = NULL;
+  }
+
+  ~scope_level ()
+  {
+    if (this->body)
+      {
+	obstack_free (this->body, NULL);
+	xfree (this->body);
+      }
+  }
+
   /* Elements we allow at this level.  */
   const struct gdb_xml_element *elements;
 
@@ -52,11 +79,9 @@ struct scope_level
      optional and repeatable checking).  */
   unsigned int seen;
 
-  /* Body text accumulation.  */
+  /* Body text accumulation.  This is an owning pointer.  */
   struct obstack *body;
 };
-typedef struct scope_level scope_level_s;
-DEF_VEC_O(scope_level_s);
 
 /* The parser itself, and our additional state.  */
 struct gdb_xml_parser
@@ -71,7 +96,8 @@ struct gdb_xml_parser
   const char *name;		/* Name of this parser.  */
   void *user_data;		/* The user's callback data, for handlers.  */
 
-  VEC(scope_level_s) *scopes;	/* Scoping stack.  */
+  /* Scoping stack.  */
+  std::vector<scope_level> scopes;
 
   struct gdb_exception error;	/* A thrown error, if any.  */
   int last_line;		/* The line of the thrown error, or 0.  */
@@ -89,18 +115,19 @@ static void
 gdb_xml_body_text (void *data, const XML_Char *text, int length)
 {
   struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data;
-  struct scope_level *scope = VEC_last (scope_level_s, parser->scopes);
 
   if (parser->error.reason < 0)
     return;
 
-  if (scope->body == NULL)
+  scope_level &scope = parser->scopes.back ();
+
+  if (scope.body == NULL)
     {
-      scope->body = XCNEW (struct obstack);
-      obstack_init (scope->body);
+      scope.body = XCNEW (struct obstack);
+      obstack_init (scope.body);
     }
 
-  obstack_grow (scope->body, text, length);
+  obstack_grow (scope.body, text, length);
 }
 
 /* Issue a debugging message from one of PARSER's handlers.  */
@@ -179,8 +206,6 @@ gdb_xml_start_element (void *data, const XML_Char *name,
 		       const XML_Char **attrs)
 {
   struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data;
-  struct scope_level *scope;
-  struct scope_level new_scope;
   const struct gdb_xml_element *element;
   const struct gdb_xml_attribute *attribute;
   VEC(gdb_xml_value_s) *attributes = NULL;
@@ -189,11 +214,16 @@ gdb_xml_start_element (void *data, const XML_Char *name,
 
   /* Push an error scope.  If we return or throw an exception before
      filling this in, it will tell us to ignore children of this
-     element.  */
-  VEC_reserve (scope_level_s, parser->scopes, 1);
-  scope = VEC_last (scope_level_s, parser->scopes);
-  memset (&new_scope, 0, sizeof (new_scope));
-  VEC_quick_push (scope_level_s, parser->scopes, &new_scope);
+     element.  Note we don't take a reference to the element yet
+     because further below we'll process the element which may recurse
+     back here and push more elements to the vector.  When the
+     recursion unrolls all such elements will have been popped back
+     already, but if one of those pushes reallocates the vector,
+     previous element references will be invalidated.  */
+  parser->scopes.emplace_back ();
+
+  /* Get a reference to the current scope.  */
+  scope_level &scope = parser->scopes[parser->scopes.size () - 2];
 
   gdb_xml_debug (parser, _("Entering element <%s>"), name);
 
@@ -201,7 +231,7 @@ gdb_xml_start_element (void *data, const XML_Char *name,
      children.  Record that we've seen it.  */
 
   seen = 1;
-  for (element = scope->elements; element && element->name;
+  for (element = scope.elements; element && element->name;
        element++, seen <<= 1)
     if (strcmp (element->name, name) == 0)
       break;
@@ -213,12 +243,10 @@ gdb_xml_start_element (void *data, const XML_Char *name,
 	 list into the new scope even if there was no match.  */
       if (parser->is_xinclude)
 	{
-	  struct scope_level *unknown_scope;
-
 	  XML_DefaultCurrent (parser->expat_parser);
 
-	  unknown_scope = VEC_last (scope_level_s, parser->scopes);
-	  unknown_scope->elements = scope->elements;
+	  scope_level &unknown_scope = parser->scopes.back ();
+	  unknown_scope.elements = scope.elements;
 	  return;
 	}
 
@@ -226,10 +254,10 @@ gdb_xml_start_element (void *data, const XML_Char *name,
       return;
     }
 
-  if (!(element->flags & GDB_XML_EF_REPEATABLE) && (seen & scope->seen))
+  if (!(element->flags & GDB_XML_EF_REPEATABLE) && (seen & scope.seen))
     gdb_xml_error (parser, _("Element <%s> only expected once"), name);
 
-  scope->seen |= seen;
+  scope.seen |= seen;
 
   back_to = make_cleanup (gdb_xml_values_cleanup, &attributes);
 
@@ -302,10 +330,13 @@ gdb_xml_start_element (void *data, const XML_Char *name,
   if (element->start_handler)
     element->start_handler (parser, element, parser->user_data, attributes);
 
-  /* Fill in a new scope level.  */
-  scope = VEC_last (scope_level_s, parser->scopes);
-  scope->element = element;
-  scope->elements = element->children;
+  /* Fill in a new scope level.  Note that we must delay getting a
+     back reference till here because above we might have recursed,
+     which may have reallocated the vector which invalidates
+     iterators/pointers/references.  */
+  scope_level &new_scope = parser->scopes.back ();
+  new_scope.element = element;
+  new_scope.elements = element->children;
 
   do_cleanups (back_to);
 }
@@ -342,7 +373,7 @@ gdb_xml_start_element_wrapper (void *data, const XML_Char *name,
 static void
 gdb_xml_end_element (gdb_xml_parser *parser, const XML_Char *name)
 {
-  struct scope_level *scope = VEC_last (scope_level_s, parser->scopes);
+  struct scope_level *scope = &parser->scopes.back ();
   const struct gdb_xml_element *element;
   unsigned int seen;
 
@@ -387,12 +418,7 @@ gdb_xml_end_element (gdb_xml_parser *parser, const XML_Char *name)
     XML_DefaultCurrent (parser->expat_parser);
 
   /* Pop the scope level.  */
-  if (scope->body)
-    {
-      obstack_free (scope->body, NULL);
-      xfree (scope->body);
-    }
-  VEC_pop (scope_level_s, parser->scopes);
+  parser->scopes.pop_back ();
 }
 
 /* Wrapper for gdb_xml_end_element, to prevent throwing exceptions
@@ -424,19 +450,7 @@ gdb_xml_end_element_wrapper (void *data, const XML_Char *name)
 
 gdb_xml_parser::~gdb_xml_parser ()
 {
-  struct scope_level *scope;
-  int ix;
-
   XML_ParserFree (this->expat_parser);
-
-  /* Clean up the scopes.  */
-  for (ix = 0; VEC_iterate (scope_level_s, this->scopes, ix, scope); ix++)
-    if (scope->body)
-      {
-	obstack_free (scope->body, NULL);
-	xfree (scope->body);
-      }
-  VEC_free (scope_level_s, this->scopes);
 }
 
 /* Initialize a parser.  */
@@ -446,7 +460,6 @@ gdb_xml_parser::gdb_xml_parser (const char *name_,
 				void *user_data_)
   : name (name_),
     user_data (user_data_),
-    scopes (NULL),
     error (exception_none),
     last_line (0),
     dtd_name (NULL),
@@ -464,10 +477,7 @@ gdb_xml_parser::gdb_xml_parser (const char *name_,
   XML_SetCharacterDataHandler (this->expat_parser, gdb_xml_body_text);
 
   /* Initialize the outer scope.  */
-  scope_level start_scope;
-  memset (&start_scope, 0, sizeof (start_scope));
-  start_scope.elements = elements;
-  VEC_safe_push (scope_level_s, this->scopes, &start_scope);
+  this->scopes.emplace_back (elements);
 }
 
 /* External entity handler.  The only external entities we support
-- 
2.5.5

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 6/7] gdb_xml_parser: make data fields private and make more functions methods
  2017-04-10 12:22 [PATCH 0/7] C++fy xml-support.c and more gdb::optional Pedro Alves
                   ` (3 preceding siblings ...)
  2017-04-10 12:22 ` [PATCH 7/7] Eliminate obstack_printf Pedro Alves
@ 2017-04-10 12:22 ` Pedro Alves
  2017-04-10 12:22 ` [PATCH 2/7] xml-support.c: Use std::vector Pedro Alves
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2017-04-10 12:22 UTC (permalink / raw)
  To: gdb-patches

This patch makes the data fields of gdb_xml_parser private, and makes
more functions be gdb_xml_parser methods.  This is mostly for better
encapsulation.

Some free functions have their parsing-related guts converted to
methods, while the free functions remain, as they're used as expat
callbacks.  Now their only job is to be small shims that restore back
the gdb_xml_parser type, defer work to the corresponding method, and
make sure C++ exceptions don't cross expat.

More C++-fycation of the XML parsers built on top of gdb_xml_parser
could follow, but this was my stopping point.

gdb/ChangeLog:
yyyy-mm-dd  Pedro Alves  <palves@redhat.com>

	* xml-support.c (gdb_xml_parser) <use_dtd, dtd_name, parse,
	vdebug, verror, body_text, start_element, end_element, name,
	user_data, set_is_xinclude, set_error, expat_parser>: New methods.
	<name, user_data, expat_parser, scopes, error, last_line, dtd_name,
	is_xinclude>: Make private and add m_ prefix.
	(gdb_xml_parser::body_text): New method, based on ...
	(gdb_xml_body_text): ... this.  Adjust.
	(gdb_xml_parser::vdebug): New method, based on ...
	(gdb_xml_debug): ... this.  Adjust.
	(gdb_xml_parser::verror): New method, based on ...
	(gdb_xml_error): ... this.  Adjust.
	(gdb_xml_parser::start_element): New method, based on ...
	(gdb_xml_start_element): ... this.  Adjust.
	(gdb_xml_start_element_wrapper): Defer to
	gdb_xml_parser::start_element and gdb_xml_parser::set_error.
	(gdb_xml_parser::end_element): New method, based on ...
	(gdb_xml_end_element_wrapper): ... this.  Adjust.
	(gdb_xml_parser::~gdb_xml_parser): Adjust.
	(gdb_xml_parser::gdb_xml_parser): Adjust to field renames.
	(gdb_xml_parser::use_dtd): New method, based on ...
	(gdb_xml_use_dtd): ... this.  Adjust.
	(gdb_xml_parser::parse): New method, based on ...
	(gdb_xml_parse): ... this.  Adjust.
	(gdb_xml_parse_quick): Adjust to call the parser's parse method.
	(xinclude_start_include): Adjust to call the parser's name method.
	(xml_xinclude_default, xml_xinclude_start_doctype)
	(xml_xinclude_end_doctype): Adjust to call the parser's user_data
	method.
	(xml_process_xincludes): Adjust to call parser methods.
	* xml-support.h (gdb_xml_use_dtd, gdb_xml_parse): Delete
	declarations.
---
 gdb/xml-support.c | 341 +++++++++++++++++++++++++++++++++---------------------
 gdb/xml-support.h |  14 ---
 2 files changed, 206 insertions(+), 149 deletions(-)

diff --git a/gdb/xml-support.c b/gdb/xml-support.c
index 91e31d9..9595246 100644
--- a/gdb/xml-support.c
+++ b/gdb/xml-support.c
@@ -72,73 +72,156 @@ struct gdb_xml_parser
 		  void *user_data);
   ~gdb_xml_parser();
 
-  XML_Parser expat_parser;	/* The underlying expat parser.  */
+  /* Associate DTD_NAME, which must be the name of a compiled-in DTD,
+     with the parser.  */
+  void use_dtd (const char *dtd_name);
+
+  /* Return the name of the expected / default DTD, if specified.  */
+  const char *dtd_name ()
+  { return m_dtd_name; }
+
+  /* Invoke the parser on BUFFER.  BUFFER is the data to parse, which
+     should be NUL-terminated.
+
+     The return value is 0 for success or -1 for error.  It may throw,
+     but only if something unexpected goes wrong during parsing; parse
+     errors will be caught, warned about, and reported as failure.  */
+  int parse (const char *buffer);
+
+  /* Issue a debugging message.  */
+  void vdebug (const char *format, va_list ap)
+    ATTRIBUTE_PRINTF (2, 0);
+
+  /* Issue an error message, and stop parsing.  */
+  void verror (const char *format, va_list ap)
+    ATTRIBUTE_NORETURN ATTRIBUTE_PRINTF (2, 0);
+
+  void body_text (const XML_Char *text, int length);
+  void start_element (const XML_Char *name, const XML_Char **attrs);
+  void end_element (const XML_Char *name);
+
+  /* Return the name of this parser.  */
+  const char *name ()
+  { return m_name; }
+
+  /* Return the user's callback data, for handlers.  */
+  void *user_data ()
+  { return m_user_data; };
+
+  /* Are we the special <xi:include> parser?  */
+  void set_is_xinclude (bool is_xinclude)
+  { m_is_xinclude = is_xinclude; }
+
+  /* A thrown error, if any.  */
+  void set_error (gdb_exception error)
+  {
+    m_error = error;
+#ifdef HAVE_XML_STOPPARSER
+    XML_StopParser (m_expat_parser, XML_FALSE);
+#endif
+  }
+
+  /* Return the underlying expat parser.  */
+  XML_Parser expat_parser ()
+  { return m_expat_parser; }
+
+private:
+  /* The underlying expat parser.  */
+  XML_Parser m_expat_parser;
+
+  /* Name of this parser.  */
+  const char *m_name;
 
-  const char *name;		/* Name of this parser.  */
-  void *user_data;		/* The user's callback data, for handlers.  */
+  /* The user's callback data, for handlers.  */
+  void *m_user_data;
 
   /* Scoping stack.  */
-  std::vector<scope_level> scopes;
+  std::vector<scope_level> m_scopes;
 
-  struct gdb_exception error;	/* A thrown error, if any.  */
-  int last_line;		/* The line of the thrown error, or 0.  */
+/* A thrown error, if any.  */
+  struct gdb_exception m_error;
 
-  const char *dtd_name;		/* The name of the expected / default DTD,
-				   if specified.  */
-  bool is_xinclude;		/* Are we the special <xi:include> parser?  */
+  /* The line of the thrown error, or 0.  */
+  int m_last_line;
+
+  /* The name of the expected / default DTD, if specified.  */
+  const char *m_dtd_name;
+
+  /* Are we the special <xi:include> parser?  */
+  bool m_is_xinclude;
 };
 
 /* Process some body text.  We accumulate the text for later use; it's
    wrong to do anything with it immediately, because a single block of
    text might be broken up into multiple calls to this function.  */
 
+void
+gdb_xml_parser::body_text (const XML_Char *text, int length)
+{
+  if (m_error.reason < 0)
+    return;
+
+  scope_level &scope = m_scopes.back ();
+  scope.body.append (text, length);
+}
+
 static void
 gdb_xml_body_text (void *data, const XML_Char *text, int length)
 {
   struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data;
 
-  if (parser->error.reason < 0)
-    return;
-
-  scope_level &scope = parser->scopes.back ();
-  scope.body.append (text, length);
+  parser->body_text (text, length);
 }
 
 /* Issue a debugging message from one of PARSER's handlers.  */
 
 void
-gdb_xml_debug (struct gdb_xml_parser *parser, const char *format, ...)
+gdb_xml_parser::vdebug (const char *format, va_list ap)
 {
-  int line = XML_GetCurrentLineNumber (parser->expat_parser);
-  va_list ap;
+  int line = XML_GetCurrentLineNumber (m_expat_parser);
   char *message;
 
-  if (!debug_xml)
-    return;
-
-  va_start (ap, format);
   message = xstrvprintf (format, ap);
   if (line)
     fprintf_unfiltered (gdb_stderr, "%s (line %d): %s\n",
-			parser->name, line, message);
+			m_name, line, message);
   else
     fprintf_unfiltered (gdb_stderr, "%s: %s\n",
-			parser->name, message);
+			m_name, message);
   xfree (message);
 }
 
+void
+gdb_xml_debug (struct gdb_xml_parser *parser, const char *format, ...)
+{
+  if (!debug_xml)
+    return;
+
+  va_list ap;
+  va_start (ap, format);
+  parser->vdebug (format, ap);
+  va_end (ap);
+}
+
 /* Issue an error message from one of PARSER's handlers, and stop
    parsing.  */
 
 void
+gdb_xml_parser::verror (const char *format, va_list ap)
+{
+  int line = XML_GetCurrentLineNumber (m_expat_parser);
+
+  m_last_line = line;
+  throw_verror (XML_PARSE_ERROR, format, ap);
+}
+
+void
 gdb_xml_error (struct gdb_xml_parser *parser, const char *format, ...)
 {
-  int line = XML_GetCurrentLineNumber (parser->expat_parser);
   va_list ap;
-
-  parser->last_line = line;
   va_start (ap, format);
-  throw_verror (XML_PARSE_ERROR, format, ap);
+  parser->verror (format, ap);
+  va_end (ap);
 }
 
 /* Find the attribute named NAME in the set of parsed attributes
@@ -171,15 +254,16 @@ gdb_xml_values_cleanup (void *data)
   VEC_free (gdb_xml_value_s, *values);
 }
 
-/* Handle the start of an element.  DATA is our local XML parser, NAME
-   is the element, and ATTRS are the names and values of this
-   element's attributes.  */
+/* Handle the start of an element.  NAME is the element, and ATTRS are
+   the names and values of this element's attributes.  */
 
-static void
-gdb_xml_start_element (void *data, const XML_Char *name,
-		       const XML_Char **attrs)
+void
+gdb_xml_parser::start_element (const XML_Char *name,
+			       const XML_Char **attrs)
 {
-  struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data;
+  if (m_error.reason < 0)
+    return;
+
   const struct gdb_xml_element *element;
   const struct gdb_xml_attribute *attribute;
   VEC(gdb_xml_value_s) *attributes = NULL;
@@ -194,12 +278,12 @@ gdb_xml_start_element (void *data, const XML_Char *name,
      recursion unrolls all such elements will have been popped back
      already, but if one of those pushes reallocates the vector,
      previous element references will be invalidated.  */
-  parser->scopes.emplace_back ();
+  m_scopes.emplace_back ();
 
   /* Get a reference to the current scope.  */
-  scope_level &scope = parser->scopes[parser->scopes.size () - 2];
+  scope_level &scope = m_scopes[m_scopes.size () - 2];
 
-  gdb_xml_debug (parser, _("Entering element <%s>"), name);
+  gdb_xml_debug (this, _("Entering element <%s>"), name);
 
   /* Find this element in the list of the current scope's allowed
      children.  Record that we've seen it.  */
@@ -215,21 +299,21 @@ gdb_xml_start_element (void *data, const XML_Char *name,
       /* If we're working on XInclude, <xi:include> can be the child
 	 of absolutely anything.  Copy the previous scope's element
 	 list into the new scope even if there was no match.  */
-      if (parser->is_xinclude)
+      if (m_is_xinclude)
 	{
-	  XML_DefaultCurrent (parser->expat_parser);
+	  XML_DefaultCurrent (m_expat_parser);
 
-	  scope_level &unknown_scope = parser->scopes.back ();
+	  scope_level &unknown_scope = m_scopes.back ();
 	  unknown_scope.elements = scope.elements;
 	  return;
 	}
 
-      gdb_xml_debug (parser, _("Element <%s> unknown"), name);
+      gdb_xml_debug (this, _("Element <%s> unknown"), name);
       return;
     }
 
   if (!(element->flags & GDB_XML_EF_REPEATABLE) && (seen & scope.seen))
-    gdb_xml_error (parser, _("Element <%s> only expected once"), name);
+    gdb_xml_error (this, _("Element <%s> only expected once"), name);
 
   scope.seen |= seen;
 
@@ -253,14 +337,14 @@ gdb_xml_start_element (void *data, const XML_Char *name,
 
       if (*p != NULL && val == NULL)
 	{
-	  gdb_xml_debug (parser, _("Attribute \"%s\" missing a value"),
+	  gdb_xml_debug (this, _("Attribute \"%s\" missing a value"),
 			 attribute->name);
 	  continue;
 	}
 
       if (*p == NULL && !(attribute->flags & GDB_XML_AF_OPTIONAL))
 	{
-	  gdb_xml_error (parser, _("Required attribute \"%s\" of "
+	  gdb_xml_error (this, _("Required attribute \"%s\" of "
 				   "<%s> not specified"),
 			 attribute->name, element->name);
 	  continue;
@@ -269,11 +353,11 @@ gdb_xml_start_element (void *data, const XML_Char *name,
       if (*p == NULL)
 	continue;
 
-      gdb_xml_debug (parser, _("Parsing attribute %s=\"%s\""),
+      gdb_xml_debug (this, _("Parsing attribute %s=\"%s\""),
 		     attribute->name, val);
 
       if (attribute->handler)
-	parsed_value = attribute->handler (parser, attribute, val);
+	parsed_value = attribute->handler (this, attribute, val);
       else
 	parsed_value = xstrdup (val);
 
@@ -296,19 +380,19 @@ gdb_xml_start_element (void *data, const XML_Char *name,
 	      break;
 
 	  if (attribute == NULL || attribute->name == NULL)
-	    gdb_xml_debug (parser, _("Ignoring unknown attribute %s"), *p);
+	    gdb_xml_debug (this, _("Ignoring unknown attribute %s"), *p);
 	}
     }
 
   /* Call the element handler if there is one.  */
   if (element->start_handler)
-    element->start_handler (parser, element, parser->user_data, attributes);
+    element->start_handler (this, element, m_user_data, attributes);
 
   /* Fill in a new scope level.  Note that we must delay getting a
      back reference till here because above we might have recursed,
      which may have reallocated the vector which invalidates
      iterators/pointers/references.  */
-  scope_level &new_scope = parser->scopes.back ();
+  scope_level &new_scope = m_scopes.back ();
   new_scope.element = element;
   new_scope.elements = element->children;
 
@@ -324,41 +408,37 @@ gdb_xml_start_element_wrapper (void *data, const XML_Char *name,
 {
   struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data;
 
-  if (parser->error.reason < 0)
-    return;
-
   TRY
     {
-      gdb_xml_start_element (data, name, attrs);
+      parser->start_element (name, attrs);
     }
   CATCH (ex, RETURN_MASK_ALL)
     {
-      parser->error = ex;
-#ifdef HAVE_XML_STOPPARSER
-      XML_StopParser (parser->expat_parser, XML_FALSE);
-#endif
+      parser->set_error (ex);
     }
   END_CATCH
 }
 
-/* Handle the end of an element.  PARSER is our local XML parser, and
-   NAME is the current element.  */
+/* Handle the end of an element.  NAME is the current element.  */
 
-static void
-gdb_xml_end_element (gdb_xml_parser *parser, const XML_Char *name)
+void
+gdb_xml_parser::end_element (const XML_Char *name)
 {
-  struct scope_level *scope = &parser->scopes.back ();
+  if (m_error.reason < 0)
+    return;
+
+  struct scope_level *scope = &m_scopes.back ();
   const struct gdb_xml_element *element;
   unsigned int seen;
 
-  gdb_xml_debug (parser, _("Leaving element <%s>"), name);
+  gdb_xml_debug (this, _("Leaving element <%s>"), name);
 
   for (element = scope->elements, seen = 1;
        element != NULL && element->name != NULL;
        element++, seen <<= 1)
     if ((scope->seen & seen) == 0
 	&& (element->flags & GDB_XML_EF_OPTIONAL) == 0)
-      gdb_xml_error (parser, _("Required element <%s> is missing"),
+      gdb_xml_error (this, _("Required element <%s> is missing"),
 		     element->name);
 
   /* Call the element processor.  */
@@ -383,14 +463,14 @@ gdb_xml_end_element (gdb_xml_parser *parser, const XML_Char *name)
 	    body++;
 	}
 
-      scope->element->end_handler (parser, scope->element, parser->user_data,
-				   body);
+      scope->element->end_handler (this, scope->element,
+				   m_user_data, body);
     }
   else if (scope->element == NULL)
-    XML_DefaultCurrent (parser->expat_parser);
+    XML_DefaultCurrent (m_expat_parser);
 
   /* Pop the scope level.  */
-  parser->scopes.pop_back ();
+  m_scopes.pop_back ();
 }
 
 /* Wrapper for gdb_xml_end_element, to prevent throwing exceptions
@@ -401,19 +481,13 @@ gdb_xml_end_element_wrapper (void *data, const XML_Char *name)
 {
   struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data;
 
-  if (parser->error.reason < 0)
-    return;
-
   TRY
     {
-      gdb_xml_end_element (parser, name);
+      parser->end_element (name);
     }
   CATCH (ex, RETURN_MASK_ALL)
     {
-      parser->error = ex;
-#ifdef HAVE_XML_STOPPARSER
-      XML_StopParser (parser->expat_parser, XML_FALSE);
-#endif
+      parser->set_error (ex);
     }
   END_CATCH
 }
@@ -422,34 +496,34 @@ gdb_xml_end_element_wrapper (void *data, const XML_Char *name)
 
 gdb_xml_parser::~gdb_xml_parser ()
 {
-  XML_ParserFree (this->expat_parser);
+  XML_ParserFree (m_expat_parser);
 }
 
 /* Initialize a parser.  */
 
-gdb_xml_parser::gdb_xml_parser (const char *name_,
+gdb_xml_parser::gdb_xml_parser (const char *name,
 				const gdb_xml_element *elements,
-				void *user_data_)
-  : name (name_),
-    user_data (user_data_),
-    error (exception_none),
-    last_line (0),
-    dtd_name (NULL),
-    is_xinclude (false)
+				void *user_data)
+  : m_name (name),
+    m_user_data (user_data),
+    m_error (exception_none),
+    m_last_line (0),
+    m_dtd_name (NULL),
+    m_is_xinclude (false)
 {
-  this->expat_parser = XML_ParserCreateNS (NULL, '!');
-  if (this->expat_parser == NULL)
+  m_expat_parser = XML_ParserCreateNS (NULL, '!');
+  if (m_expat_parser == NULL)
     malloc_failure (0);
 
-  XML_SetUserData (this->expat_parser, this);
+  XML_SetUserData (m_expat_parser, this);
 
   /* Set the callbacks.  */
-  XML_SetElementHandler (this->expat_parser, gdb_xml_start_element_wrapper,
+  XML_SetElementHandler (m_expat_parser, gdb_xml_start_element_wrapper,
 			 gdb_xml_end_element_wrapper);
-  XML_SetCharacterDataHandler (this->expat_parser, gdb_xml_body_text);
+  XML_SetCharacterDataHandler (m_expat_parser, gdb_xml_body_text);
 
   /* Initialize the outer scope.  */
-  this->scopes.emplace_back (elements);
+  m_scopes.emplace_back (elements);
 }
 
 /* External entity handler.  The only external entities we support
@@ -463,19 +537,20 @@ gdb_xml_fetch_external_entity (XML_Parser expat_parser,
 			       const XML_Char *systemId,
 			       const XML_Char *publicId)
 {
-  struct gdb_xml_parser *parser
-    = (struct gdb_xml_parser *) XML_GetUserData (expat_parser);
   XML_Parser entity_parser;
   const char *text;
   enum XML_Status status;
 
   if (systemId == NULL)
     {
-      text = fetch_xml_builtin (parser->dtd_name);
+      gdb_xml_parser *parser
+	= (gdb_xml_parser *) XML_GetUserData (expat_parser);
+
+      text = fetch_xml_builtin (parser->dtd_name ());
       if (text == NULL)
 	internal_error (__FILE__, __LINE__,
 			_("could not locate built-in DTD %s"),
-			parser->dtd_name);
+			parser->dtd_name ());
     }
   else
     {
@@ -484,7 +559,8 @@ gdb_xml_fetch_external_entity (XML_Parser expat_parser,
 	return XML_STATUS_ERROR;
     }
 
-  entity_parser = XML_ExternalEntityParserCreate (expat_parser, context, NULL);
+  entity_parser = XML_ExternalEntityParserCreate (expat_parser,
+						  context, NULL);
 
   /* Don't use our handlers for the contents of the DTD.  Just let expat
      process it.  */
@@ -500,23 +576,20 @@ gdb_xml_fetch_external_entity (XML_Parser expat_parser,
   return status;
 }
 
-/* Associate DTD_NAME, which must be the name of a compiled-in DTD,
-   with PARSER.  */
-
 void
-gdb_xml_use_dtd (struct gdb_xml_parser *parser, const char *dtd_name)
+gdb_xml_parser::use_dtd (const char *dtd_name)
 {
   enum XML_Error err;
 
-  parser->dtd_name = dtd_name;
+  m_dtd_name = dtd_name;
 
-  XML_SetParamEntityParsing (parser->expat_parser,
+  XML_SetParamEntityParsing (m_expat_parser,
 			     XML_PARAM_ENTITY_PARSING_UNLESS_STANDALONE);
-  XML_SetExternalEntityRefHandler (parser->expat_parser,
+  XML_SetExternalEntityRefHandler (m_expat_parser,
 				   gdb_xml_fetch_external_entity);
 
   /* Even if no DTD is provided, use the built-in DTD anyway.  */
-  err = XML_UseForeignDTD (parser->expat_parser, XML_TRUE);
+  err = XML_UseForeignDTD (m_expat_parser, XML_TRUE);
   if (err != XML_ERROR_NONE)
     internal_error (__FILE__, __LINE__,
 		    _("XML_UseForeignDTD failed: %s"),
@@ -531,41 +604,41 @@ gdb_xml_use_dtd (struct gdb_xml_parser *parser, const char *dtd_name)
    errors will be caught, warned about, and reported as failure.  */
 
 int
-gdb_xml_parse (struct gdb_xml_parser *parser, const char *buffer)
+gdb_xml_parser::parse (const char *buffer)
 {
   enum XML_Status status;
   const char *error_string;
 
-  gdb_xml_debug (parser, _("Starting:\n%s"), buffer);
+  gdb_xml_debug (this, _("Starting:\n%s"), buffer);
 
-  status = XML_Parse (parser->expat_parser, buffer, strlen (buffer), 1);
+  status = XML_Parse (m_expat_parser, buffer, strlen (buffer), 1);
 
-  if (status == XML_STATUS_OK && parser->error.reason == 0)
+  if (status == XML_STATUS_OK && m_error.reason == 0)
     return 0;
 
-  if (parser->error.reason == RETURN_ERROR
-      && parser->error.error == XML_PARSE_ERROR)
+  if (m_error.reason == RETURN_ERROR
+      && m_error.error == XML_PARSE_ERROR)
     {
-      gdb_assert (parser->error.message != NULL);
-      error_string = parser->error.message;
+      gdb_assert (m_error.message != NULL);
+      error_string = m_error.message;
     }
   else if (status == XML_STATUS_ERROR)
     {
-      enum XML_Error err = XML_GetErrorCode (parser->expat_parser);
+      enum XML_Error err = XML_GetErrorCode (m_expat_parser);
 
       error_string = XML_ErrorString (err);
     }
   else
     {
-      gdb_assert (parser->error.reason < 0);
-      throw_exception (parser->error);
+      gdb_assert (m_error.reason < 0);
+      throw_exception (m_error);
     }
 
-  if (parser->last_line != 0)
-    warning (_("while parsing %s (at line %d): %s"), parser->name,
-	     parser->last_line, error_string);
+  if (m_last_line != 0)
+    warning (_("while parsing %s (at line %d): %s"), m_name,
+	     m_last_line, error_string);
   else
-    warning (_("while parsing %s: %s"), parser->name, error_string);
+    warning (_("while parsing %s: %s"), m_name, error_string);
 
   return -1;
 }
@@ -577,8 +650,8 @@ gdb_xml_parse_quick (const char *name, const char *dtd_name,
 {
   gdb_xml_parser parser (name, elements, user_data);
   if (dtd_name != NULL)
-    gdb_xml_use_dtd (&parser, dtd_name);
-  return gdb_xml_parse (&parser, document);
+    parser.use_dtd (dtd_name);
+  return parser.parse (document);
 }
 
 /* Parse a field VALSTR that we expect to contain an integer value.
@@ -749,7 +822,8 @@ xinclude_start_include (struct gdb_xml_parser *parser,
     gdb_xml_error (parser, _("Could not load XML document \"%s\""), href);
   back_to = make_cleanup (xfree, text);
 
-  if (!xml_process_xincludes (data->output, parser->name, text, data->fetcher,
+  if (!xml_process_xincludes (data->output, parser->name (),
+			      text, data->fetcher,
 			      data->fetcher_baton,
 			      data->include_depth + 1))
     gdb_xml_error (parser, _("Parsing \"%s\" failed"), href);
@@ -774,8 +848,7 @@ static void XMLCALL
 xml_xinclude_default (void *data_, const XML_Char *s, int len)
 {
   struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data_;
-  struct xinclude_parsing_data *data
-    = (struct xinclude_parsing_data *) parser->user_data;
+  xinclude_parsing_data *data = (xinclude_parsing_data *) parser->user_data ();
 
   /* If we are inside of e.g. xi:include or the DTD, don't save this
      string.  */
@@ -793,8 +866,7 @@ xml_xinclude_start_doctype (void *data_, const XML_Char *doctypeName,
 			    int has_internal_subset)
 {
   struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data_;
-  struct xinclude_parsing_data *data
-    = (struct xinclude_parsing_data *) parser->user_data;
+  xinclude_parsing_data *data = (xinclude_parsing_data *) parser->user_data ();
 
   /* Don't print out the doctype, or the contents of the DTD internal
      subset, if any.  */
@@ -805,8 +877,7 @@ static void XMLCALL
 xml_xinclude_end_doctype (void *data_)
 {
   struct gdb_xml_parser *parser = (struct gdb_xml_parser *) data_;
-  struct xinclude_parsing_data *data
-    = (struct xinclude_parsing_data *) parser->user_data;
+  xinclude_parsing_data *data = (xinclude_parsing_data *) parser->user_data ();
 
   data->skip_depth--;
 }
@@ -843,25 +914,25 @@ xml_process_xincludes (std::string &result,
   xinclude_parsing_data data (result, fetcher, fetcher_baton, depth);
 
   gdb_xml_parser parser (name, xinclude_elements, &data);
-  parser.is_xinclude = true;
+  parser.set_is_xinclude (true);
 
-  XML_SetCharacterDataHandler (parser.expat_parser, NULL);
-  XML_SetDefaultHandler (parser.expat_parser, xml_xinclude_default);
+  XML_SetCharacterDataHandler (parser.expat_parser (), NULL);
+  XML_SetDefaultHandler (parser.expat_parser (), xml_xinclude_default);
 
   /* Always discard the XML version declarations; the only important
      thing this provides is encoding, and our result will have been
      converted to UTF-8.  */
-  XML_SetXmlDeclHandler (parser.expat_parser, xml_xinclude_xml_decl);
+  XML_SetXmlDeclHandler (parser.expat_parser (), xml_xinclude_xml_decl);
 
   if (depth > 0)
     /* Discard the doctype for included documents.  */
-    XML_SetDoctypeDeclHandler (parser.expat_parser,
+    XML_SetDoctypeDeclHandler (parser.expat_parser (),
 			       xml_xinclude_start_doctype,
 			       xml_xinclude_end_doctype);
 
-  gdb_xml_use_dtd (&parser, "xinclude.dtd");
+  parser.use_dtd ("xinclude.dtd");
 
-  if (gdb_xml_parse (&parser, text) == 0)
+  if (parser.parse (text) == 0)
     {
       if (depth == 0)
 	gdb_xml_debug (&parser, _("XInclude processing succeeded."));
diff --git a/gdb/xml-support.h b/gdb/xml-support.h
index 8248a32..f9ea64d 100644
--- a/gdb/xml-support.h
+++ b/gdb/xml-support.h
@@ -172,20 +172,6 @@ struct gdb_xml_element
   gdb_xml_element_end_handler *end_handler;
 };
 
-/* Associate DTD_NAME, which must be the name of a compiled-in DTD,
-   with PARSER.  */
-
-void gdb_xml_use_dtd (struct gdb_xml_parser *parser, const char *dtd_name);
-
-/* Invoke PARSER on BUFFER.  BUFFER is the data to parse, which
-   should be NUL-terminated.
-
-   The return value is 0 for success or -1 for error.  It may throw,
-   but only if something unexpected goes wrong during parsing; parse
-   errors will be caught, warned about, and reported as failure.  */
-
-int gdb_xml_parse (struct gdb_xml_parser *parser, const char *buffer);
-
 /* Parse a XML document.  DOCUMENT is the data to parse, which should
    be NUL-terminated. If non-NULL, use the compiled-in DTD named
    DTD_NAME to drive the parsing.
-- 
2.5.5

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 4/7] gdb::optional unit tests
  2017-04-10 12:22 [PATCH 0/7] C++fy xml-support.c and more gdb::optional Pedro Alves
                   ` (5 preceding siblings ...)
  2017-04-10 12:22 ` [PATCH 2/7] xml-support.c: Use std::vector Pedro Alves
@ 2017-04-10 12:32 ` Pedro Alves
  2017-04-15 16:20 ` [PATCH 0/7] C++fy xml-support.c and more gdb::optional Simon Marchi
  7 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2017-04-10 12:32 UTC (permalink / raw)
  To: gdb-patches

I thought I'd add some unit tests to make sure gdb::optional behaved
correctly, and started writing some, but then thought/realized that
libstdc++ already has extensive testing for C++17 std::optional, which
gdb::optional is a subset of, and thought why bother writing something
from scratch.  So I tried copying over a subset of libstdc++'s tests
(that ones that cover the subset supported by gdb::optional), and was
positively surprised that they mostly work OOTB.  This did help shake
out a few bugs from what I was implementing in the previous patch to
gdb::optional.  Still, it's a good chunk of code being copied over, so
if people dislike this copying/duplication, I can drop this patch.

gdb/ChangeLog:
2017-04-07  Pedro Alves  <palves@redhat.com>

	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/optional-selftests.c.
	(SUBDIR_UNITTESTS_OBS): Add optional-selftests.o.
	* unittests/optional-selftests.c: New file.
	* unittests/optional/assignment/1.cc: New file.
	* unittests/optional/assignment/2.cc: New file.
	* unittests/optional/assignment/3.cc: New file.
	* unittests/optional/assignment/4.cc: New file.
	* unittests/optional/assignment/5.cc: New file.
	* unittests/optional/assignment/6.cc: New file.
	* unittests/optional/assignment/7.cc: New file.
	* unittests/optional/cons/copy.cc: New file.
	* unittests/optional/cons/default.cc: New file.
	* unittests/optional/cons/move.cc: New file.
	* unittests/optional/cons/value.cc: New file.
	* unittests/optional/in_place.cc: New file.
	* unittests/optional/observers/1.cc: New file.
	* unittests/optional/observers/2.cc: New file.
---
 gdb/Makefile.in                        |   6 +-
 gdb/unittests/optional-selftests.c     |  94 +++++++++++
 gdb/unittests/optional/assignment/1.cc | 195 ++++++++++++++++++++++
 gdb/unittests/optional/assignment/2.cc | 193 ++++++++++++++++++++++
 gdb/unittests/optional/assignment/3.cc | 156 +++++++++++++++++
 gdb/unittests/optional/assignment/4.cc | 156 +++++++++++++++++
 gdb/unittests/optional/assignment/5.cc |  80 +++++++++
 gdb/unittests/optional/assignment/6.cc |  90 ++++++++++
 gdb/unittests/optional/assignment/7.cc |  29 ++++
 gdb/unittests/optional/cons/copy.cc    | 126 ++++++++++++++
 gdb/unittests/optional/cons/default.cc |  58 +++++++
 gdb/unittests/optional/cons/move.cc    | 124 ++++++++++++++
 gdb/unittests/optional/cons/value.cc   | 294 +++++++++++++++++++++++++++++++++
 gdb/unittests/optional/in_place.cc     |  65 ++++++++
 gdb/unittests/optional/observers/1.cc  |  31 ++++
 gdb/unittests/optional/observers/2.cc  |  35 ++++
 16 files changed, 1730 insertions(+), 2 deletions(-)
 create mode 100644 gdb/unittests/optional-selftests.c
 create mode 100644 gdb/unittests/optional/assignment/1.cc
 create mode 100644 gdb/unittests/optional/assignment/2.cc
 create mode 100644 gdb/unittests/optional/assignment/3.cc
 create mode 100644 gdb/unittests/optional/assignment/4.cc
 create mode 100644 gdb/unittests/optional/assignment/5.cc
 create mode 100644 gdb/unittests/optional/assignment/6.cc
 create mode 100644 gdb/unittests/optional/assignment/7.cc
 create mode 100644 gdb/unittests/optional/cons/copy.cc
 create mode 100644 gdb/unittests/optional/cons/default.cc
 create mode 100644 gdb/unittests/optional/cons/move.cc
 create mode 100644 gdb/unittests/optional/cons/value.cc
 create mode 100644 gdb/unittests/optional/in_place.cc
 create mode 100644 gdb/unittests/optional/observers/1.cc
 create mode 100644 gdb/unittests/optional/observers/2.cc

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 23e4bed..92dbff0 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -526,12 +526,14 @@ SUBDIR_PYTHON_CFLAGS =
 SUBDIR_UNITTESTS_SRCS = \
 	unittests/function-view-selftests.c \
 	unittests/offset-type-selftests.c \
-	unittests/ptid-selftests.c
+	unittests/ptid-selftests.c \
+	unittests/optional-selftests.c
 
 SUBDIR_UNITTESTS_OBS = \
 	function-view-selftests.o \
 	offset-type-selftests.o \
-	ptid-selftests.o
+	ptid-selftests.o \
+	optional-selftests.o
 
 # Opcodes currently live in one of two places.  Either they are in the
 # opcode library, typically ../opcodes, or they are in a header file
diff --git a/gdb/unittests/optional-selftests.c b/gdb/unittests/optional-selftests.c
new file mode 100644
index 0000000..76343c6
--- /dev/null
+++ b/gdb/unittests/optional-selftests.c
@@ -0,0 +1,94 @@
+/* Self tests for optional for GDB, the GNU debugger.
+
+   Copyright (C) 2017 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "selftest.h"
+#include "common/gdb_optional.h"
+
+/* Used by the included .cc files below.  Included here because the
+   included test files are wrapped in a namespace.  */
+#include <vector>
+#include <string>
+#include <memory>
+
+/* libstdc++'s testsuite uses VERIFY.  */
+#define VERIFY SELF_CHECK
+
+/* Used to disable testing features not supported by
+   gdb::optional.  */
+#define GDB_OPTIONAL
+
+namespace selftests {
+namespace optional {
+
+/* The actual tests live in separate files, which were originally
+   copied over from libstdc++'s testsuite.  To preserve the structure
+   and help with comparison with the original tests, the file names
+   have been preserved, and only minimal modification was done to have
+   them compile against gdb::optional instead of std::optional:
+
+     - std::optional->gdb:optional, etc.
+     - ATTRIBUTE_UNUSED in a few places
+     - wrap each file in a namespace so they can all be compiled as a
+       single unit.
+     - libstdc++'s license and formatting style was preserved.
+*/
+
+#include "optional/assignment/1.cc"
+#include "optional/assignment/2.cc"
+#include "optional/assignment/3.cc"
+#include "optional/assignment/4.cc"
+#include "optional/assignment/5.cc"
+#include "optional/assignment/6.cc"
+#include "optional/assignment/7.cc"
+#include "optional/cons/copy.cc"
+#include "optional/cons/default.cc"
+#include "optional/cons/move.cc"
+#include "optional/cons/value.cc"
+#include "optional/in_place.cc"
+#include "optional/observers/1.cc"
+#include "optional/observers/2.cc"
+
+static void
+run_tests ()
+{
+  assign_1::test ();
+  assign_2::test ();
+  assign_3::test ();
+  assign_4::test ();
+  assign_5::test ();
+  assign_6::test ();
+  assign_7::test ();
+  cons_copy::test ();
+  cons_default::test ();
+  cons_move::test ();
+  cons_value::test ();
+  in_place::test ();
+  observers_1::test ();
+  observers_2::test ();
+}
+
+} /* namespace optional */
+} /* namespace selftests */
+
+void
+_initialize_optional_selftests ()
+{
+  register_self_test (selftests::optional::run_tests);
+}
diff --git a/gdb/unittests/optional/assignment/1.cc b/gdb/unittests/optional/assignment/1.cc
new file mode 100644
index 0000000..671004e
--- /dev/null
+++ b/gdb/unittests/optional/assignment/1.cc
@@ -0,0 +1,195 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace assign_1 {
+
+struct exception {};
+
+int counter = 0;
+
+struct mixin_counter
+{
+  mixin_counter() { ++counter; }
+  mixin_counter(mixin_counter const&) { ++counter; }
+  ~mixin_counter() { --counter; }
+};
+
+struct value_type : private mixin_counter
+{
+  enum state_type
+  {
+    zero,
+    moved_from,
+    throwing_construction,
+    throwing_copy,
+    throwing_copy_assignment,
+    throwing_move,
+    throwing_move_assignment,
+    threw,
+  };
+
+  value_type() = default;
+
+  explicit value_type(state_type state_)
+  : state(state_)
+  {
+    throw_if(throwing_construction);
+  }
+
+  value_type(value_type const& other)
+  : state(other.state)
+  {
+    throw_if(throwing_copy);
+  }
+
+  value_type&
+  operator=(value_type const& other)
+  {
+    state = other.state;
+    throw_if(throwing_copy_assignment);
+    return *this;
+  }
+
+  value_type(value_type&& other)
+  : state(other.state)
+  {
+    other.state = moved_from;
+    throw_if(throwing_move);
+  }
+
+  value_type&
+  operator=(value_type&& other)
+  {
+    state = other.state;
+    other.state = moved_from;
+    throw_if(throwing_move_assignment);
+    return *this;
+  }
+
+  void throw_if(state_type match)
+  {
+    if(state == match)
+    {
+      state = threw;
+      throw exception {};
+    }
+  }
+
+  state_type state = zero;
+};
+
+void test()
+{
+  using O = gdb::optional<value_type>;
+  using S = value_type::state_type;
+  auto const make = [](S s = S::zero) { return O { gdb::in_place, s }; };
+
+  enum outcome_type { nothrow, caught, bad_catch };
+
+  // Check copy/move assignment for disengaged optional
+
+  // From disengaged optional
+  {
+    O o;
+    VERIFY( !o );
+    O p;
+    o = p;
+    VERIFY( !o );
+    VERIFY( !p );
+  }
+
+  {
+    O o;
+    VERIFY( !o );
+    O p;
+    o = std::move(p);
+    VERIFY( !o );
+    VERIFY( !p );
+  }
+
+#ifndef GDB_OPTIONAL
+  {
+    O o;
+    VERIFY( !o );
+    o = {};
+    VERIFY( !o );
+  }
+#endif
+
+  // From engaged optional
+  {
+    O o;
+    VERIFY( !o );
+    O p = make(S::throwing_copy_assignment);
+    o = p;
+    VERIFY( o && o->state == S::throwing_copy_assignment );
+    VERIFY( p && p->state == S::throwing_copy_assignment );
+  }
+
+  {
+    O o;
+    VERIFY( !o );
+    O p = make(S::throwing_move_assignment);
+    o = std::move(p);
+    VERIFY( o && o->state == S::throwing_move_assignment );
+    VERIFY( p && p->state == S::moved_from );
+  }
+
+  {
+    outcome_type outcome {};
+    O o;
+    VERIFY( !o );
+    O p = make(S::throwing_copy);
+
+    try
+    {
+      o = p;
+    }
+    catch(exception const&)
+    { outcome = caught; }
+    catch(...)
+    { outcome = bad_catch; }
+
+    VERIFY( outcome == caught );
+    VERIFY( !o );
+    VERIFY( p && p->state == S::throwing_copy );
+  }
+
+  {
+    outcome_type outcome {};
+    O o;
+    VERIFY( !o );
+    O p = make(S::throwing_move);
+
+    try
+    {
+      o = std::move(p);
+    }
+    catch(exception const&)
+    { outcome = caught; }
+    catch(...)
+    { outcome = bad_catch; }
+
+    VERIFY( outcome == caught );
+    VERIFY( !o );
+    VERIFY( p && p->state == S::moved_from );
+  }
+
+  VERIFY( counter == 0 );
+}
+
+} // namespace assign_1
diff --git a/gdb/unittests/optional/assignment/2.cc b/gdb/unittests/optional/assignment/2.cc
new file mode 100644
index 0000000..1b0bd7a
--- /dev/null
+++ b/gdb/unittests/optional/assignment/2.cc
@@ -0,0 +1,193 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace assign_2 {
+
+struct exception {};
+
+int counter = 0;
+
+struct mixin_counter
+{
+  mixin_counter() { ++counter; }
+  mixin_counter(mixin_counter const&) { ++counter; }
+  ~mixin_counter() { --counter; }
+};
+
+struct value_type : private mixin_counter
+{
+  enum state_type
+  {
+    zero,
+    moved_from,
+    throwing_construction,
+    throwing_copy,
+    throwing_copy_assignment,
+    throwing_move,
+    throwing_move_assignment,
+    threw,
+  };
+
+  value_type() = default;
+
+  explicit value_type(state_type state_)
+  : state(state_)
+  {
+    throw_if(throwing_construction);
+  }
+
+  value_type(value_type const& other)
+  : state(other.state)
+  {
+    throw_if(throwing_copy);
+  }
+
+  value_type&
+  operator=(value_type const& other)
+  {
+    state = other.state;
+    throw_if(throwing_copy_assignment);
+    return *this;
+  }
+
+  value_type(value_type&& other)
+  : state(other.state)
+  {
+    other.state = moved_from;
+    throw_if(throwing_move);
+  }
+
+  value_type&
+  operator=(value_type&& other)
+  {
+    state = other.state;
+    other.state = moved_from;
+    throw_if(throwing_move_assignment);
+    return *this;
+  }
+
+  void throw_if(state_type match)
+  {
+    if(state == match)
+    {
+      state = threw;
+      throw exception {};
+    }
+  }
+
+  state_type state = zero;
+};
+
+void test()
+{
+  using O = gdb::optional<value_type>;
+  using S = value_type::state_type;
+  auto const make = [](S s = S::zero) { return O { gdb::in_place, s }; };
+
+  enum outcome_type { nothrow, caught, bad_catch };
+
+  // Check copy/move assignment for engaged optional
+
+  // From disengaged optional
+  {
+    O o = make(S::zero);
+    VERIFY( o );
+    O p;
+    o = p;
+    VERIFY( !o );
+    VERIFY( !p );
+  }
+
+  {
+    O o = make(S::zero);
+    VERIFY( o );
+    O p;
+    o = std::move(p);
+    VERIFY( !o );
+    VERIFY( !p );
+  }
+
+#ifndef GDB_OPTIONAL
+  {
+    O o = make(S::zero);
+    VERIFY( o );
+    o = {};
+    VERIFY( !o );
+  }
+#endif
+
+  // From engaged optional
+  {
+    O o = make(S::zero);
+    VERIFY( o );
+    O p = make(S::throwing_copy);
+    o = p;
+    VERIFY( o && o->state == S::throwing_copy);
+    VERIFY( p && p->state == S::throwing_copy);
+  }
+
+  {
+    O o = make(S::zero);
+    VERIFY( o );
+    O p = make(S::throwing_move);
+    o = std::move(p);
+    VERIFY( o && o->state == S::throwing_move);
+    VERIFY( p && p->state == S::moved_from);
+  }
+
+  {
+    ATTRIBUTE_UNUSED outcome_type outcome {};
+    O o = make(S::zero);
+    VERIFY( o );
+    O p = make(S::throwing_copy_assignment);
+
+    try
+    {
+      o = p;
+    }
+    catch(exception const&)
+    { outcome = caught; }
+    catch(...)
+    { outcome = bad_catch; }
+
+    VERIFY( o && o->state == S::threw);
+    VERIFY( p && p->state == S::throwing_copy_assignment);
+  }
+
+  {
+    ATTRIBUTE_UNUSED outcome_type outcome {};
+    O o = make(S::zero);
+    VERIFY( o );
+    O p = make(S::throwing_move_assignment);
+
+    try
+    {
+      o = std::move(p);
+    }
+    catch(exception const&)
+    { outcome = caught; }
+    catch(...)
+    { outcome = bad_catch; }
+
+    VERIFY( o && o->state == S::threw);
+    VERIFY( p && p->state == S::moved_from);
+  }
+
+  VERIFY( counter == 0 );
+}
+
+} // namespace assign_2
diff --git a/gdb/unittests/optional/assignment/3.cc b/gdb/unittests/optional/assignment/3.cc
new file mode 100644
index 0000000..e047e74
--- /dev/null
+++ b/gdb/unittests/optional/assignment/3.cc
@@ -0,0 +1,156 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace assign_3 {
+
+struct exception {};
+
+int counter = 0;
+
+struct mixin_counter
+{
+  mixin_counter() { ++counter; }
+  mixin_counter(mixin_counter const&) { ++counter; }
+  ~mixin_counter() { --counter; }
+};
+
+struct value_type : private mixin_counter
+{
+  enum state_type
+  {
+    zero,
+    moved_from,
+    throwing_construction,
+    throwing_copy,
+    throwing_copy_assignment,
+    throwing_move,
+    throwing_move_assignment,
+    threw,
+  };
+
+  value_type() = default;
+
+  explicit value_type(state_type state_)
+  : state(state_)
+  {
+    throw_if(throwing_construction);
+  }
+
+  value_type(value_type const& other)
+  : state(other.state)
+  {
+    throw_if(throwing_copy);
+  }
+
+  value_type&
+  operator=(value_type const& other)
+  {
+    state = other.state;
+    throw_if(throwing_copy_assignment);
+    return *this;
+  }
+
+  value_type(value_type&& other)
+  : state(other.state)
+  {
+    other.state = moved_from;
+    throw_if(throwing_move);
+  }
+
+  value_type&
+  operator=(value_type&& other)
+  {
+    state = other.state;
+    other.state = moved_from;
+    throw_if(throwing_move_assignment);
+    return *this;
+  }
+
+  void throw_if(state_type match)
+  {
+    if(state == match)
+    {
+      state = threw;
+      throw exception {};
+    }
+  }
+
+  state_type state = zero;
+};
+
+void test()
+{
+  using O = gdb::optional<value_type>;
+  using S = value_type::state_type;
+  auto const make = [](S s = S::zero) { return value_type { s }; };
+
+  enum outcome_type { nothrow, caught, bad_catch };
+
+  // Check value assignment for disengaged optional
+
+  {
+    O o;
+    value_type v = make(S::throwing_copy_assignment);
+    o = v;
+    VERIFY( o && o->state == S::throwing_copy_assignment );
+  }
+
+  {
+    O o;
+    value_type v = make(S::throwing_move_assignment);
+    o = std::move(v);
+    VERIFY( o && o->state == S::throwing_move_assignment );
+  }
+
+  {
+    ATTRIBUTE_UNUSED outcome_type outcome {};
+    O o;
+    value_type v = make(S::throwing_copy);
+
+    try
+    {
+      o = v;
+    }
+    catch(exception const&)
+    { outcome = caught; }
+    catch(...)
+    { outcome = bad_catch; }
+
+    VERIFY( !o );
+  }
+
+  {
+    ATTRIBUTE_UNUSED outcome_type outcome {};
+    O o;
+    value_type v = make(S::throwing_move);
+
+    try
+    {
+      o = std::move(v);
+    }
+    catch(exception const&)
+    { outcome = caught; }
+    catch(...)
+    { outcome = bad_catch; }
+
+    VERIFY( !o );
+  }
+
+  VERIFY( counter == 0 );
+}
+
+} // namespace assign_3
diff --git a/gdb/unittests/optional/assignment/4.cc b/gdb/unittests/optional/assignment/4.cc
new file mode 100644
index 0000000..0b196e0
--- /dev/null
+++ b/gdb/unittests/optional/assignment/4.cc
@@ -0,0 +1,156 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace assign_4 {
+
+struct exception {};
+
+int counter = 0;
+
+struct mixin_counter
+{
+  mixin_counter() { ++counter; }
+  mixin_counter(mixin_counter const&) { ++counter; }
+  ~mixin_counter() { --counter; }
+};
+
+struct value_type : private mixin_counter
+{
+  enum state_type
+  {
+    zero,
+    moved_from,
+    throwing_construction,
+    throwing_copy,
+    throwing_copy_assignment,
+    throwing_move,
+    throwing_move_assignment,
+    threw,
+  };
+
+  value_type() = default;
+
+  explicit value_type(state_type state_)
+  : state(state_)
+  {
+    throw_if(throwing_construction);
+  }
+
+  value_type(value_type const& other)
+  : state(other.state)
+  {
+    throw_if(throwing_copy);
+  }
+
+  value_type&
+  operator=(value_type const& other)
+  {
+    state = other.state;
+    throw_if(throwing_copy_assignment);
+    return *this;
+  }
+
+  value_type(value_type&& other)
+  : state(other.state)
+  {
+    other.state = moved_from;
+    throw_if(throwing_move);
+  }
+
+  value_type&
+  operator=(value_type&& other)
+  {
+    state = other.state;
+    other.state = moved_from;
+    throw_if(throwing_move_assignment);
+    return *this;
+  }
+
+  void throw_if(state_type match)
+  {
+    if(state == match)
+    {
+      state = threw;
+      throw exception {};
+    }
+  }
+
+  state_type state = zero;
+};
+
+void test()
+{
+  using O = gdb::optional<value_type>;
+  using S = value_type::state_type;
+  auto const make = [](S s = S::zero) { return value_type { s }; };
+
+  enum outcome_type { nothrow, caught, bad_catch };
+
+  // Check value assignment for engaged optional
+
+  {
+    O o = make();
+    value_type v = make(S::throwing_copy);
+    o = v;
+    VERIFY( o && o->state == S::throwing_copy);
+  }
+
+  {
+    O o = make();
+    value_type v = make(S::throwing_move);
+    o = std::move(v);
+    VERIFY( o && o->state == S::throwing_move);
+  }
+
+  {
+    ATTRIBUTE_UNUSED outcome_type outcome {};
+    O o = make();
+    value_type v = make(S::throwing_copy_assignment);
+
+    try
+    {
+      o = v;
+    }
+    catch(exception const&)
+    { outcome = caught; }
+    catch(...)
+    { outcome = bad_catch; }
+
+    VERIFY( o && o->state == S::threw );
+  }
+
+  {
+    ATTRIBUTE_UNUSED outcome_type outcome {};
+    O o = make();
+    value_type v = make(S::throwing_move_assignment);
+
+    try
+    {
+      o = std::move(v);
+    }
+    catch(exception const&)
+    { outcome = caught; }
+    catch(...)
+    { outcome = bad_catch; }
+
+    VERIFY( o && o->state == S::threw );
+  }
+
+  VERIFY( counter == 0 );
+}
+
+} // namespace assign_4
diff --git a/gdb/unittests/optional/assignment/5.cc b/gdb/unittests/optional/assignment/5.cc
new file mode 100644
index 0000000..b1dee4f
--- /dev/null
+++ b/gdb/unittests/optional/assignment/5.cc
@@ -0,0 +1,80 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace assign_5 {
+
+int counter = 0;
+
+struct mixin_counter
+{
+  mixin_counter() { ++counter; }
+  mixin_counter(mixin_counter const&) { ++counter; }
+  ~mixin_counter() { --counter; }
+};
+
+struct value_type : private mixin_counter { };
+
+void test()
+{
+  using O = gdb::optional<value_type>;
+
+  // Check std::nullopt_t and 'default' (= {}) assignment
+
+#ifndef GDB_OPTIONAL
+  {
+    O o;
+    o = std::nullopt;
+    VERIFY( !o );
+  }
+#endif
+
+#ifndef GDB_OPTIONAL
+  {
+    O o { gdb::in_place };
+    o = std::nullopt;
+    VERIFY( !o );
+  }
+#endif
+
+#ifndef GDB_OPTIONAL
+  {
+    O o;
+    o = {};
+    VERIFY( !o );
+  }
+#endif
+
+#ifndef GDB_OPTIONAL
+  {
+    O o { gdb::in_place };
+    o = {};
+    VERIFY( !o );
+  }
+#endif
+  {
+    gdb::optional<std::vector<int>> ovi{{1, 2, 3}};
+    VERIFY(ovi->size() == 3);
+    VERIFY((*ovi)[0] == 1 && (*ovi)[1] == 2 && (*ovi)[2] == 3);
+    ovi = {4, 5, 6, 7};
+    VERIFY(ovi->size() == 4);
+    VERIFY((*ovi)[0] == 4 && (*ovi)[1] == 5 &&
+	   (*ovi)[2] == 6 && (*ovi)[3] == 7);
+  }
+  VERIFY( counter == 0 );
+}
+
+} // namespace assign_5
diff --git a/gdb/unittests/optional/assignment/6.cc b/gdb/unittests/optional/assignment/6.cc
new file mode 100644
index 0000000..383ff7e
--- /dev/null
+++ b/gdb/unittests/optional/assignment/6.cc
@@ -0,0 +1,90 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace assign_6 {
+
+int counter = 0;
+
+struct mixin_counter
+{
+  mixin_counter() { ++counter; }
+  mixin_counter(mixin_counter const&) { ++counter; }
+  ~mixin_counter() { --counter; }
+};
+
+struct value_type : private mixin_counter
+{
+  value_type() = default;
+  value_type(int) : state(1) { }
+  value_type(std::initializer_list<char>, const char*) : state(2) { }
+  int state = 0;
+};
+
+void test()
+{
+  using O = gdb::optional<value_type>;
+
+  // Check emplace
+
+  {
+    O o;
+    o.emplace();
+    VERIFY( o && o->state == 0 );
+  }
+  {
+    O o { gdb::in_place, 0 };
+    o.emplace();
+    VERIFY( o && o->state == 0 );
+  }
+
+  {
+    O o;
+    o.emplace(0);
+    VERIFY( o && o->state == 1 );
+  }
+  {
+    O o { gdb::in_place };
+    o.emplace(0);
+    VERIFY( o && o->state == 1 );
+  }
+
+#ifndef GDB_OPTIONAL
+  {
+    O o;
+    o.emplace({ 'a' }, "");
+    VERIFY( o && o->state == 2 );
+  }
+  {
+    O o { gdb::in_place };
+    o.emplace({ 'a' }, "");
+    VERIFY( o && o->state == 2 );
+  }
+#endif
+  {
+    O o;
+    VERIFY(&o.emplace(0) == &*o);
+#ifndef GDB_OPTIONAL
+    VERIFY(&o.emplace({ 'a' }, "") == &*o);
+#endif
+  }
+
+  static_assert( !std::is_constructible<O, std::initializer_list<int>, int>(), "" );
+
+  VERIFY( counter == 0 );
+}
+
+} // namespace assign_6
diff --git a/gdb/unittests/optional/assignment/7.cc b/gdb/unittests/optional/assignment/7.cc
new file mode 100644
index 0000000..e23651c
--- /dev/null
+++ b/gdb/unittests/optional/assignment/7.cc
@@ -0,0 +1,29 @@
+// Copyright (C) 2016-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace assign_7 {
+
+void test()
+{
+  gdb::optional<int> o{666};
+  VERIFY(o && *o == 666);
+  o.reset();
+  VERIFY(!o);
+  static_assert(noexcept(std::declval<gdb::optional<int>>().reset()), "");
+}
+
+} // namespace assign_7
diff --git a/gdb/unittests/optional/cons/copy.cc b/gdb/unittests/optional/cons/copy.cc
new file mode 100644
index 0000000..bce423b
--- /dev/null
+++ b/gdb/unittests/optional/cons/copy.cc
@@ -0,0 +1,126 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace cons_copy {
+
+struct tracker
+{
+  tracker(int value) : value(value) { ++count; }
+  ~tracker() { --count; }
+
+  tracker(tracker const& other) : value(other.value) { ++count; }
+  tracker(tracker&& other) : value(other.value)
+  {
+    other.value = -1;
+    ++count;
+  }
+
+  tracker& operator=(tracker const&) = default;
+  tracker& operator=(tracker&&) = default;
+
+  int value;
+
+  static int count;
+};
+
+int tracker::count = 0;
+
+struct exception { };
+
+struct throwing_copy
+{
+  throwing_copy() = default;
+  throwing_copy(throwing_copy const&) { throw exception {}; }
+};
+
+void test()
+{
+  // [20.5.4.1] Constructors
+
+  {
+    gdb::optional<long> o;
+    auto copy = o;
+    VERIFY( !copy );
+    VERIFY( !o );
+  }
+
+  {
+    const long val = 0x1234ABCD;
+    gdb::optional<long> o { gdb::in_place, val};
+    auto copy = o;
+    VERIFY( copy );
+    VERIFY( *copy == val );
+#ifndef GDB_OPTIONAL
+    VERIFY( o && o == val );
+#endif
+  }
+
+  {
+    gdb::optional<tracker> o;
+    auto copy = o;
+    VERIFY( !copy );
+    VERIFY( tracker::count == 0 );
+    VERIFY( !o );
+  }
+
+  {
+    gdb::optional<tracker> o { gdb::in_place, 333 };
+    auto copy = o;
+    VERIFY( copy );
+    VERIFY( copy->value == 333 );
+    VERIFY( tracker::count == 2 );
+    VERIFY( o && o->value == 333 );
+  }
+
+  enum outcome { nothrow, caught, bad_catch };
+
+  {
+    outcome result = nothrow;
+    gdb::optional<throwing_copy> o;
+
+    try
+    {
+      auto copy = o;
+    }
+    catch(exception const&)
+    { result = caught; }
+    catch(...)
+    { result = bad_catch; }
+
+    VERIFY( result == nothrow );
+  }
+
+  {
+    outcome result = nothrow;
+    gdb::optional<throwing_copy> o { gdb::in_place };
+
+    try
+    {
+      auto copy = o;
+    }
+    catch(exception const&)
+    { result = caught; }
+    catch(...)
+    { result = bad_catch; }
+
+    VERIFY( result == caught );
+  }
+
+  VERIFY( tracker::count == 0 );
+}
+
+} // namespace cons_copy
diff --git a/gdb/unittests/optional/cons/default.cc b/gdb/unittests/optional/cons/default.cc
new file mode 100644
index 0000000..b075f5c
--- /dev/null
+++ b/gdb/unittests/optional/cons/default.cc
@@ -0,0 +1,58 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace cons_default {
+
+struct tracker
+{
+  tracker() { ++count; }
+  ~tracker() { --count; }
+
+  tracker(tracker const&) { ++count; }
+  tracker(tracker&&) { ++count; }
+
+  tracker& operator=(tracker const&) = default;
+  tracker& operator=(tracker&&) = default;
+
+  static int count;
+};
+
+int tracker::count = 0;
+
+void test()
+{
+  // [20.5.4.1] Constructors
+
+  {
+    gdb::optional<tracker> o;
+    VERIFY( !o );
+  }
+
+  {
+    gdb::optional<tracker> o {};
+    VERIFY( !o );
+  }
+
+  {
+    gdb::optional<tracker> o = {};
+    VERIFY( !o );
+  }
+
+  VERIFY( tracker::count == 0 );
+}
+
+} // namespace cons_default
diff --git a/gdb/unittests/optional/cons/move.cc b/gdb/unittests/optional/cons/move.cc
new file mode 100644
index 0000000..5a50b08
--- /dev/null
+++ b/gdb/unittests/optional/cons/move.cc
@@ -0,0 +1,124 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace cons_move {
+
+struct tracker
+{
+  tracker(int value) : value(value) { ++count; }
+  ~tracker() { --count; }
+
+  tracker(tracker const& other) : value(other.value) { ++count; }
+  tracker(tracker&& other) : value(other.value)
+  {
+    other.value = -1;
+    ++count;
+  }
+
+  tracker& operator=(tracker const&) = default;
+  tracker& operator=(tracker&&) = default;
+
+  int value;
+
+  static int count;
+};
+
+int tracker::count = 0;
+
+struct exception { };
+
+struct throwing_move
+{
+  throwing_move() = default;
+  throwing_move(throwing_move const&) { throw exception {}; }
+};
+
+void test()
+{
+  // [20.5.4.1] Constructors
+
+  {
+    gdb::optional<long> o;
+    auto moved_to = std::move(o);
+    VERIFY( !moved_to );
+    VERIFY( !o );
+  }
+
+  {
+    const long val = 0x1234ABCD;
+    gdb::optional<long> o { gdb::in_place, val};
+    auto moved_to = std::move(o);
+    VERIFY( moved_to );
+    VERIFY( *moved_to == val );
+    VERIFY( o && *o == val );
+  }
+
+  {
+    gdb::optional<tracker> o;
+    auto moved_to = std::move(o);
+    VERIFY( !moved_to );
+    VERIFY( tracker::count == 0 );
+    VERIFY( !o );
+  }
+
+  {
+    gdb::optional<tracker> o { gdb::in_place, 333 };
+    auto moved_to = std::move(o);
+    VERIFY( moved_to );
+    VERIFY( moved_to->value == 333 );
+    VERIFY( tracker::count == 2 );
+    VERIFY( o && o->value == -1 );
+  }
+
+  enum outcome { nothrow, caught, bad_catch };
+
+  {
+    outcome result = nothrow;
+    gdb::optional<throwing_move> o;
+
+    try
+    {
+      auto moved_to = std::move(o);
+    }
+    catch(exception const&)
+    { result = caught; }
+    catch(...)
+    { result = bad_catch; }
+
+    VERIFY( result == nothrow );
+  }
+
+  {
+    outcome result = nothrow;
+    gdb::optional<throwing_move> o { gdb::in_place };
+
+    try
+    {
+      auto moved_to = std::move(o);
+    }
+    catch(exception const&)
+    { result = caught; }
+    catch(...)
+    { result = bad_catch; }
+
+    VERIFY( result == caught );
+  }
+
+  VERIFY( tracker::count == 0 );
+}
+
+} // namespace cons_move
diff --git a/gdb/unittests/optional/cons/value.cc b/gdb/unittests/optional/cons/value.cc
new file mode 100644
index 0000000..52843b0
--- /dev/null
+++ b/gdb/unittests/optional/cons/value.cc
@@ -0,0 +1,294 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace cons_value {
+
+struct tracker
+{
+  tracker(int value) : value(value) { ++count; }
+  ~tracker() { --count; }
+
+  tracker(tracker const& other) : value(other.value) { ++count; }
+  tracker(tracker&& other) : value(other.value)
+  {
+    other.value = -1;
+    ++count;
+  }
+
+  tracker& operator=(tracker const&) = default;
+  tracker& operator=(tracker&&) = default;
+
+  int value;
+
+  static int count;
+};
+
+int tracker::count = 0;
+
+struct exception { };
+
+struct throwing_construction
+{
+  explicit throwing_construction(bool propagate) : propagate(propagate) { }
+
+  throwing_construction(throwing_construction const& other)
+  : propagate(other.propagate)
+  {
+    if(propagate)
+      throw exception {};
+  }
+
+  bool propagate;
+};
+
+void test()
+{
+  // [20.5.4.1] Constructors
+
+  {
+    auto i = 0x1234ABCD;
+    gdb::optional<long> o { i };
+    VERIFY( o );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
+  }
+
+  {
+    auto i = 0x1234ABCD;
+    gdb::optional<long> o = i;
+    VERIFY( o );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
+  }
+
+  {
+    auto i = 0x1234ABCD;
+    gdb::optional<long> o = { i };
+    VERIFY( o );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
+  }
+
+  {
+    auto i = 0x1234ABCD;
+    gdb::optional<long> o { std::move(i) };
+    VERIFY( o );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
+  }
+
+  {
+    auto i = 0x1234ABCD;
+    gdb::optional<long> o = std::move(i);
+    VERIFY( o );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
+  }
+
+  {
+    auto i = 0x1234ABCD;
+    gdb::optional<long> o = { std::move(i) };
+    VERIFY( o );
+    VERIFY( *o == 0x1234ABCD );
+    VERIFY( i == 0x1234ABCD );
+  }
+
+#ifndef GDB_OPTIONAL
+  {
+    std::vector<int> v = { 0, 1, 2, 3, 4, 5 };
+    gdb::optional<std::vector<int>> o { v };
+    VERIFY( !v.empty() );
+    VERIFY( o->size() == 6 );
+  }
+#endif
+
+  {
+    std::vector<int> v = { 0, 1, 2, 3, 4, 5 };
+    gdb::optional<std::vector<int>> o = v;
+    VERIFY( !v.empty() );
+    VERIFY( o->size() == 6 );
+  }
+
+  {
+    std::vector<int> v = { 0, 1, 2, 3, 4, 5 };
+    gdb::optional<std::vector<int>> o { v };
+    VERIFY( !v.empty() );
+    VERIFY( o->size() == 6 );
+  }
+
+  {
+    std::vector<int> v = { 0, 1, 2, 3, 4, 5 };
+    gdb::optional<std::vector<int>> o { std::move(v) };
+    VERIFY( v.empty() );
+    VERIFY( o->size() == 6 );
+  }
+
+  {
+    std::vector<int> v = { 0, 1, 2, 3, 4, 5 };
+    gdb::optional<std::vector<int>> o = std::move(v);
+    VERIFY( v.empty() );
+    VERIFY( o->size() == 6 );
+  }
+
+  {
+    std::vector<int> v = { 0, 1, 2, 3, 4, 5 };
+    gdb::optional<std::vector<int>> o { std::move(v) };
+    VERIFY( v.empty() );
+    VERIFY( o->size() == 6 );
+  }
+
+  {
+    tracker t { 333 };
+    gdb::optional<tracker> o = t;
+    VERIFY( o->value == 333 );
+    VERIFY( tracker::count == 2 );
+    VERIFY( t.value == 333 );
+  }
+
+  {
+    tracker t { 333 };
+    gdb::optional<tracker> o = std::move(t);
+    VERIFY( o->value == 333 );
+    VERIFY( tracker::count == 2 );
+    VERIFY( t.value == -1 );
+  }
+
+  enum outcome { nothrow, caught, bad_catch };
+
+  {
+    outcome result = nothrow;
+    throwing_construction t { false };
+
+    try
+    {
+      gdb::optional<throwing_construction> o { t };
+    }
+    catch(exception const&)
+    { result = caught; }
+    catch(...)
+    { result = bad_catch; }
+
+    VERIFY( result == nothrow );
+  }
+
+  {
+    outcome result = nothrow;
+    throwing_construction t { true };
+
+    try
+    {
+      gdb::optional<throwing_construction> o { t };
+    }
+    catch(exception const&)
+    { result = caught; }
+    catch(...)
+    { result = bad_catch; }
+
+    VERIFY( result == caught );
+  }
+
+  {
+    outcome result = nothrow;
+    throwing_construction t { false };
+
+    try
+    {
+      gdb::optional<throwing_construction> o { std::move(t) };
+    }
+    catch(exception const&)
+    { result = caught; }
+    catch(...)
+    { result = bad_catch; }
+
+    VERIFY( result == nothrow );
+  }
+
+  {
+    outcome result = nothrow;
+    throwing_construction t { true };
+
+    try
+    {
+      gdb::optional<throwing_construction> o { std::move(t) };
+    }
+    catch(exception const&)
+    { result = caught; }
+    catch(...)
+    { result = bad_catch; }
+
+    VERIFY( result == caught );
+  }
+
+  {
+#ifndef GDB_OPTIONAL
+    gdb::optional<std::string> os = "foo";
+#endif
+    struct X
+    {
+      explicit X(int) {}
+      X& operator=(int) {return *this;}
+    };
+#ifndef GDB_OPTIONAL
+    gdb::optional<X> ox{42};
+#endif
+    gdb::optional<int> oi{42};
+#ifndef GDB_OPTIONAL
+    gdb::optional<X> ox2{oi};
+#endif
+    gdb::optional<std::string> os2;
+    os2 = "foo";
+#ifndef GDB_OPTIONAL
+    gdb::optional<X> ox3;
+    ox3 = 42;
+    gdb::optional<X> ox4;
+    ox4 = oi;
+#endif
+  }
+  {
+    // no converting construction.
+#ifndef GDB_OPTIONAL
+    gdb::optional<int> oi = gdb::optional<short>();
+    VERIFY(!bool(oi));
+    gdb::optional<std::string> os = gdb::optional<const char*>();
+    VERIFY(!bool(os));
+#endif
+    gdb::optional<gdb::optional<int>> ooi = gdb::optional<int>();
+    VERIFY(bool(ooi));
+    ooi = gdb::optional<int>();
+    VERIFY(bool(ooi));
+    ooi = gdb::optional<int>(42);
+    VERIFY(bool(ooi));
+    VERIFY(bool(*ooi));
+#ifndef GDB_OPTIONAL
+    gdb::optional<gdb::optional<int>> ooi2 = gdb::optional<short>();
+    VERIFY(bool(ooi2));
+    ooi2 = gdb::optional<short>();
+    VERIFY(bool(ooi2));
+    ooi2 = gdb::optional<short>(6);
+    VERIFY(bool(ooi2));
+    VERIFY(bool(*ooi2));
+    gdb::optional<gdb::optional<int>> ooi3 = gdb::optional<int>(42);
+    VERIFY(bool(ooi3));
+    VERIFY(bool(*ooi3));
+    gdb::optional<gdb::optional<int>> ooi4 = gdb::optional<short>(6);
+    VERIFY(bool(ooi4));
+    VERIFY(bool(*ooi4));
+#endif
+  }
+}
+
+} // namespace cons_value
diff --git a/gdb/unittests/optional/in_place.cc b/gdb/unittests/optional/in_place.cc
new file mode 100644
index 0000000..5ddaece
--- /dev/null
+++ b/gdb/unittests/optional/in_place.cc
@@ -0,0 +1,65 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace in_place {
+
+void test()
+{
+  // [20.5.5] In-place construction
+  {
+    gdb::optional<int> o { gdb::in_place };
+    VERIFY( o );
+    VERIFY( *o == int() );
+
+#ifndef GDB_OPTIONAL
+    static_assert( !std::is_convertible<gdb::in_place_t, gdb::optional<int>>(), "" );
+#endif
+  }
+
+  {
+    gdb::optional<int> o { gdb::in_place, 42 };
+    VERIFY( o );
+    VERIFY( *o == 42 );
+  }
+
+  {
+    gdb::optional<std::vector<int>> o { gdb::in_place, 18, 4 };
+    VERIFY( o );
+    VERIFY( o->size() == 18 );
+    VERIFY( (*o)[17] == 4 );
+  }
+
+#ifndef GDB_OPTIONAL
+  {
+    gdb::optional<std::vector<int>> o { gdb::in_place, { 18, 4 } };
+    VERIFY( o );
+    VERIFY( o->size() == 2 );
+    VERIFY( (*o)[0] == 18 );
+  }
+#endif
+
+#ifndef GDB_OPTIONAL
+  {
+    gdb::optional<std::vector<int>> o { gdb::in_place, { 18, 4 }, std::allocator<int> {} };
+    VERIFY( o );
+    VERIFY( o->size() == 2 );
+    VERIFY( (*o)[0] == 18 );
+  }
+#endif
+}
+
+} // namespace in_place
diff --git a/gdb/unittests/optional/observers/1.cc b/gdb/unittests/optional/observers/1.cc
new file mode 100644
index 0000000..6c8677e
--- /dev/null
+++ b/gdb/unittests/optional/observers/1.cc
@@ -0,0 +1,31 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace observers_1 {
+
+struct value_type
+{
+  int i;
+};
+
+void test()
+{
+  gdb::optional<value_type> o { value_type { 51 } };
+  VERIFY( (*o).i == 51 );
+}
+
+} // namespace observers_1
diff --git a/gdb/unittests/optional/observers/2.cc b/gdb/unittests/optional/observers/2.cc
new file mode 100644
index 0000000..3af49e6
--- /dev/null
+++ b/gdb/unittests/optional/observers/2.cc
@@ -0,0 +1,35 @@
+// Copyright (C) 2013-2017 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+
+namespace observers_2 {
+
+struct value_type
+{
+  int i;
+};
+
+void* operator&(const value_type&) = delete;
+
+void test()
+{
+  gdb::optional<value_type> o { value_type { 51 } };
+  VERIFY( o->i == 51 );
+  VERIFY( o->i == (*o).i );
+  VERIFY( &o->i == &(*o).i );
+}
+
+} // namespace observers_2
-- 
2.5.5

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/7] xml-support.c: Use std::string for growing string  buffer
  2017-04-10 12:22 ` [PATCH 5/7] xml-support.c: Use std::string for growing string buffer Pedro Alves
@ 2017-04-15  4:36   ` Simon Marchi
  2017-04-15  6:03     ` Simon Marchi
  0 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2017-04-15  4:36 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On 2017-04-10 08:22, Pedro Alves wrote:
> This main idea behind this patch is this change to 
> xml-support.c:scope_level
> 
>   -  /* Body text accumulation.  This is an owning pointer.  */
>   -  struct obstack *body;
>   +  /* Body text accumulation.  */
>   +  std::string body;
> 
> ... which allows simplifying other parts of the code.
> 
> In target_fetch_description_xml, we want to distinguish between
> returning "success + empty std::string" and "no success", and
> gdb::optional is a natural fit for that.

When growing a string like that, isn't it more efficient to use a 
stringstream?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/7] xml-support.c: Use std::string for growing string   buffer
  2017-04-15  4:36   ` Simon Marchi
@ 2017-04-15  6:03     ` Simon Marchi
  0 siblings, 0 replies; 13+ messages in thread
From: Simon Marchi @ 2017-04-15  6:03 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On 2017-04-15 00:36, Simon Marchi wrote:
> On 2017-04-10 08:22, Pedro Alves wrote:
>> This main idea behind this patch is this change to 
>> xml-support.c:scope_level
>> 
>>   -  /* Body text accumulation.  This is an owning pointer.  */
>>   -  struct obstack *body;
>>   +  /* Body text accumulation.  */
>>   +  std::string body;
>> 
>> ... which allows simplifying other parts of the code.
>> 
>> In target_fetch_description_xml, we want to distinguish between
>> returning "success + empty std::string" and "no success", and
>> gdb::optional is a natural fit for that.
> 
> When growing a string like that, isn't it more efficient to use a 
> stringstream?

Nevermind, I think I got confused with Java, where string objects are 
immutable.  So every time you do an append, it actually allocates a new 
string object.  It's therefore good practice to use a StringBuilder in 
those cases.  The C++ string works similarly to a vector (with 
exponential capacity growth), so the number of reallocations is not bad.

Simon

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/7] C++fy xml-support.c and more gdb::optional
  2017-04-10 12:22 [PATCH 0/7] C++fy xml-support.c and more gdb::optional Pedro Alves
                   ` (6 preceding siblings ...)
  2017-04-10 12:32 ` [PATCH 4/7] gdb::optional unit tests Pedro Alves
@ 2017-04-15 16:20 ` Simon Marchi
  2017-04-18 22:58   ` Pedro Alves
  7 siblings, 1 reply; 13+ messages in thread
From: Simon Marchi @ 2017-04-15 16:20 UTC (permalink / raw)
  To: Pedro Alves; +Cc: gdb-patches

On 2017-04-10 08:22, Pedro Alves wrote:
> I've been sitting on this series for a while now as part of the whole
> "eliminate cleanups" effort, and since other folks are looking at
> eliminating cleanups too, I thought I'd get this one over with to
> avoid duplication.
> 
> In one of the patches (and also in [1] and in another larger series
> I'm working on) I kept wanting to use gdb::optional, but finding
> "opt.emplace(....)" a bit cumbersome compared to the more natural
> ctor/op=.  So I bit the bullet and brought in a bit more of C++17's
> std::optional API into gdb::optional.
> 
> I then tried to follow the "Unit tests or it didn't happen (TM)"
> principle and added unit tests for gdb::optional.  That payed off and
> caught some bugs in the new code I was adding.
> 
> [1] - https://sourceware.org/ml/gdb-patches/2017-03/msg00497.html
> 
> Tested on x86-64 Fedora 23, native and gdbserver.

I looked at the series at a high level, it looks good.  I don't mind 
about the tests, they can only be helpful.

I am actually happy about the change to optional, because I recently 
tried to do a function that returned an optional and found out it wasn't 
possible yet.

Thanks,

Simon

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/7] C++fy xml-support.c and more gdb::optional
  2017-04-15 16:20 ` [PATCH 0/7] C++fy xml-support.c and more gdb::optional Simon Marchi
@ 2017-04-18 22:58   ` Pedro Alves
  0 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2017-04-18 22:58 UTC (permalink / raw)
  To: Simon Marchi; +Cc: gdb-patches

On 04/15/2017 05:20 PM, Simon Marchi wrote:
> On 2017-04-10 08:22, Pedro Alves wrote:
>> I've been sitting on this series for a while now as part of the whole
>> "eliminate cleanups" effort, and since other folks are looking at
>> eliminating cleanups too, I thought I'd get this one over with to
>> avoid duplication.
>>
>> In one of the patches (and also in [1] and in another larger series
>> I'm working on) I kept wanting to use gdb::optional, but finding
>> "opt.emplace(....)" a bit cumbersome compared to the more natural
>> ctor/op=.  So I bit the bullet and brought in a bit more of C++17's
>> std::optional API into gdb::optional.
>>
>> I then tried to follow the "Unit tests or it didn't happen (TM)"
>> principle and added unit tests for gdb::optional.  That payed off and
>> caught some bugs in the new code I was adding.
>>
>> [1] - https://sourceware.org/ml/gdb-patches/2017-03/msg00497.html
>>
>> Tested on x86-64 Fedora 23, native and gdbserver.
> 
> I looked at the series at a high level, it looks good.  I don't mind
> about the tests, they can only be helpful.

Thanks, I've pushed it in now.

> I am actually happy about the change to optional, because I recently
> tried to do a function that returned an optional and found out it wasn't
> possible yet.

Oh, that's right.  I should have mentioned that in the commit log.
I've done that now.  I'll reply to that patch with with I pushed.

Thanks,
Pedro Alves

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/7] More gdb::optional features
  2017-04-10 12:22 ` [PATCH 3/7] More gdb::optional features Pedro Alves
@ 2017-04-18 23:00   ` Pedro Alves
  0 siblings, 0 replies; 13+ messages in thread
From: Pedro Alves @ 2017-04-18 23:00 UTC (permalink / raw)
  To: GDB Patches

This is what I pushed.  It's the same patch, but with commit
log tweaked to mention the issue with returning gdb::optional
from functions.

From 22796e972f18c5601cecb0251222411a352836b6 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 18 Apr 2017 21:39:24 +0100
Subject: [PATCH] More gdb::optional features
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently we can't use gdb::optional<T> as function return type,
because gdb::optional's copy ctor is deleted.  For example, with:

  gdb::optional<int> function ()
  {
    gdb::optional<int> opt;
    ....
    return opt;

we get:

  src/gdb/foo.c: In function ‘gdb::optional<int> foo()’:
  src/gdb/foo.c:75:10: error: use of deleted function ‘gdb::optional<T>::optional(const gdb::optional<T>&) [with T = int]’
     return opt;
	    ^
  In file included from src/gdb/foo.c:68:0:
  src/gdb/common/gdb_optional.h:53:3: note: declared here
     optional (const optional &other) = delete;
     ^

I started by fixing that, and then ran into another missing feature,
also fixed by this patch.

The next feature I'm missing most from gdb::optional<T> compared to
std::optional<T> is construction/move/assignment from a T, instead of
having to default construct an gdb::optional and then use
optional::emplace(....).

For example:
  gdb::optional<std::string> function ()
  {
    gdb::optional<std::string> opt;
    std::string str;
    ...
    opt.emplace (std::move (str));
    return opt;
vs
  gdb::optional<std::string> function ()
  {
    std::string str;
    ...
    return str;

The copy/move ctor/assign methods weren't initialy implemented because
std::optional supports construction from a type U if U is convertible
to T too, and has rules to decide whether the ctors are
explicit/implicit based on that, and rules for whether the ctor should
be trivial or not, etc., which leads to a much more complicated
implementation.

If we stick to supporting copy/move construction/assignment of/to an
optional<T> from exactly only optional<T> and T, then all that
conversion-related complication disappears, and we still gain
convenience in most use cases.

The patch also makes emplace return a reference to the constructor
object, per C++17 std::optional, and adds a reset method, againt
because std::optional has one and it's trivial to support it.  These
two changes are a requirement of the gdb::optional unit testing patch
that will follow.

gdb/ChangeLog:
2017-04-18  Pedro Alves  <palves@redhat.com>

	* common/gdb_optional.h: Include common/traits.h.
	(in_place_t): New type.
	(in_place): New constexpr variable.
	(optional::optional): Remove member initialization of
	m_instantiated.
	(optional::optional(in_place_t...)): New constructor.
	(optional::~optional): Use reset.
	(optional::optional(const optional&)): New.
	(optional::optional(const optional&&)): New.
	(optional::optional(T &)): New.
	(optional::optional(T &&)): New.
	(operator::operator=(const optional &)): New.
	(operator::operator=(optional &&)): New.
	(operator::operator= (const T &))
	(operator::operator= (T &&))
	(operator::emplace (Args &&... args)): Return a T&.  Use reset.
	(operator::reset): New.
	(operator::m_instantiated):: Add in-class initializer.
	* common/traits.h: Include <type_traits>.
	(struct And): New types.
---
 gdb/ChangeLog             |  23 ++++++++
 gdb/common/gdb_optional.h | 132 ++++++++++++++++++++++++++++++++++++++++------
 gdb/common/traits.h       |  26 +++++++++
 3 files changed, 166 insertions(+), 15 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 85ab1ae..f000112 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,28 @@
 2017-04-18  Pedro Alves  <palves@redhat.com>
 
+	* common/gdb_optional.h: Include common/traits.h.
+	(in_place_t): New type.
+	(in_place): New constexpr variable.
+	(optional::optional): Remove member initialization of
+	m_instantiated.
+	(optional::optional(in_place_t...)): New constructor.
+	(optional::~optional): Use reset.
+	(optional::optional(const optional&)): New.
+	(optional::optional(const optional&&)): New.
+	(optional::optional(T &)): New.
+	(optional::optional(T &&)): New.
+	(operator::operator=(const optional &)): New.
+	(operator::operator=(optional &&)): New.
+	(operator::operator= (const T &))
+	(operator::operator= (T &&))
+	(operator::emplace (Args &&... args)): Return a T&.  Use reset.
+	(operator::reset): New.
+	(operator::m_instantiated):: Add in-class initializer.
+	* common/traits.h: Include <type_traits>.
+	(struct And): New types.
+
+2017-04-18  Pedro Alves  <palves@redhat.com>
+
 	* xml-support.c: Include <vector>.
 	(scope_level::scope_level(const gdb_xml_element *))
 	(scope_level::scope_level(scope_level&&)): New.
diff --git a/gdb/common/gdb_optional.h b/gdb/common/gdb_optional.h
index ad1119f..c30fa5b 100644
--- a/gdb/common/gdb_optional.h
+++ b/gdb/common/gdb_optional.h
@@ -20,46 +20,141 @@
 #ifndef GDB_OPTIONAL_H
 #define GDB_OPTIONAL_H
 
+#include "common/traits.h"
+
 namespace gdb
 {
 
+struct in_place_t
+{
+  explicit in_place_t () = default;
+};
+
+constexpr gdb::in_place_t in_place {};
+
 /* 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.  */
+   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.  */
+
 template<typename T>
 class optional
 {
 public:
 
   constexpr optional ()
-    : m_dummy (),
-      m_instantiated (false)
+    : m_dummy ()
+  {}
+
+  template<typename... Args>
+  constexpr optional (in_place_t, Args &&... args)
+    : m_item (std::forward<Args> (args)...),
+      m_instantiated (true)
+  {}
+
+  ~optional ()
+  { this->reset (); }
+
+  /* Copy and move constructors.  */
+
+  optional (const optional &other)
   {
+    if (other.m_instantiated)
+      this->emplace (other.get ());
   }
 
-  ~optional ()
+  optional (optional &&other)
+    noexcept(std::is_nothrow_move_constructible<T> ())
+  {
+    if (other.m_instantiated)
+      this->emplace (std::move (other.get ()));
+  }
+
+  constexpr optional (const T &other)
+    : m_item (other),
+      m_instantiated (true)
+  {}
+
+  constexpr optional (T &&other)
+    noexcept (std::is_nothrow_move_constructible<T> ())
+    : m_item (std::move (other)),
+      m_instantiated (true)
+  {}
+
+  /* Assignment operators.  */
+
+  optional &
+  operator= (const optional &other)
+  {
+    if (m_instantiated && other.m_instantiated)
+      this->get () = other.get ();
+    else
+      {
+	if (other.m_instantiated)
+	  this->emplace (other.get ());
+	else
+	  this->reset ();
+      }
+
+    return *this;
+  }
+
+  optional &
+  operator= (optional &&other)
+    noexcept (And<std::is_nothrow_move_constructible<T>,
+	      std::is_nothrow_move_assignable<T>> ())
+  {
+    if (m_instantiated && other.m_instantiated)
+      this->get () = std::move (other.get ());
+    else
+      {
+	if (other.m_instantiated)
+	  this->emplace (std::move (other.get ()));
+	else
+	  this->reset ();
+      }
+    return *this;
+  }
+
+  optional &
+  operator= (const T &other)
   {
     if (m_instantiated)
-      destroy ();
+      this->get () = other;
+    else
+      this->emplace (other);
+    return *this;
   }
 
-  /* These aren't deleted in std::optional, but it was simpler to
-     delete them here, because currently the users of this class don't
-     need them, and making them depend on the definition of T is
-     somewhat complicated.  */
-  optional (const optional &other) = delete;
-  optional<T> &operator= (const optional &other) = delete;
+  optional &
+  operator= (T &&other)
+    noexcept (And<std::is_nothrow_move_constructible<T>,
+	      std::is_nothrow_move_assignable<T>> ())
+  {
+    if (m_instantiated)
+      this->get () = std::move (other);
+    else
+      this->emplace (std::move (other));
+    return *this;
+  }
 
   template<typename... Args>
-  void emplace (Args &&... args)
+  T &emplace (Args &&... args)
   {
-    if (m_instantiated)
-      destroy ();
+    this->reset ();
     new (&m_item) T (std::forward<Args>(args)...);
     m_instantiated = true;
+    return this->get ();
   }
 
   /* Observers.  */
@@ -87,6 +182,13 @@ public:
   constexpr bool has_value () const noexcept
   { return m_instantiated; }
 
+  /* 'reset' is a 'safe' operation with no precondition.  */
+  void reset () noexcept
+  {
+    if (m_instantiated)
+      this->destroy ();
+  }
+
 private:
 
   /* Destroy the object.  */
@@ -109,7 +211,7 @@ private:
   };
 
   /* True if the object was ever emplaced.  */
-  bool m_instantiated;
+  bool m_instantiated = false;
 };
 
 }
diff --git a/gdb/common/traits.h b/gdb/common/traits.h
index 4b7bac3..c8f29cc 100644
--- a/gdb/common/traits.h
+++ b/gdb/common/traits.h
@@ -18,6 +18,8 @@
 #ifndef COMMON_TRAITS_H
 #define COMMON_TRAITS_H
 
+#include <type_traits>
+
 namespace gdb {
 
 /* Pre C++14-safe (CWG 1558) version of C++17's std::void_t.  See
@@ -29,6 +31,30 @@ struct make_void { typedef void type; };
 template<typename... Ts>
 using void_t = typename make_void<Ts...>::type;
 
+/* A few trait helpers, mainly stolen from libstdc++.  Uppercase
+   because "and" is a keyword.  */
+
+template<typename...>
+struct And;
+
+template<>
+struct And<> : public std::true_type
+{};
+
+template<typename B1>
+struct And<B1> : public B1
+{};
+
+template<typename B1, typename B2>
+struct And<B1, B2>
+  : public std::conditional<B1::value, B2, B1>::type
+{};
+
+template<typename B1, typename B2, typename B3, typename... Bn>
+struct And<B1, B2, B3, Bn...>
+  : public std::conditional<B1::value, And<B2, B3, Bn...>, B1>::type
+{};
+
 }
 
 #endif /* COMMON_TRAITS_H */
-- 
2.5.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-04-18 23:00 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-10 12:22 [PATCH 0/7] C++fy xml-support.c and more gdb::optional Pedro Alves
2017-04-10 12:22 ` [PATCH 5/7] xml-support.c: Use std::string for growing string buffer Pedro Alves
2017-04-15  4:36   ` Simon Marchi
2017-04-15  6:03     ` Simon Marchi
2017-04-10 12:22 ` [PATCH 3/7] More gdb::optional features Pedro Alves
2017-04-18 23:00   ` Pedro Alves
2017-04-10 12:22 ` [PATCH 1/7] C++-ify gdb/xml-support.c a bit to eliminate cleanups Pedro Alves
2017-04-10 12:22 ` [PATCH 7/7] Eliminate obstack_printf Pedro Alves
2017-04-10 12:22 ` [PATCH 6/7] gdb_xml_parser: make data fields private and make more functions methods Pedro Alves
2017-04-10 12:22 ` [PATCH 2/7] xml-support.c: Use std::vector Pedro Alves
2017-04-10 12:32 ` [PATCH 4/7] gdb::optional unit tests Pedro Alves
2017-04-15 16:20 ` [PATCH 0/7] C++fy xml-support.c and more gdb::optional Simon Marchi
2017-04-18 22:58   ` Pedro Alves

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).