[libxml++] Parser: Replace ExtraParserData by private Pimpl struct

[Kjell Ahlstedt <kjellahl src gnome org>]

commit 03e96a1ae6c5665d0ed745e3b0cd1f08333e525f
Author: Kjell Ahlstedt <kjell ahlstedt bredband net>
Date:   Mon Sep 21 09:08:10 2015 +0200

    Parser: Replace ExtraParserData by private Pimpl struct
    
    * libxml++/parsers/parser.[h|cc]: Replace the ExtraParserData struct in
    parser.cc by a private Parser::Pimpl struct. Move some protected data to
    Pimpl. Add pure virtual parse_memory_raw(). Default in DOM parser is to
    throw both parse errors and validity errors in exceptions.
    * libxml++/parsers/domparser.h:
    * libxml++/parsers/saxparser.h: Add override to parse_memory_raw().
    * libxml++/parsers/saxparser.cc: Call set_throw_messages(false) in ctor.
    Bug #754673.

 libxml++/parsers/domparser.h  |    2 +-
 libxml++/parsers/parser.cc    |  173 +++++++++++++++++------------------------
 libxml++/parsers/parser.h     |   68 ++++++++--------
 libxml++/parsers/saxparser.cc |    3 +
 libxml++/parsers/saxparser.h  |    2 +-
 5 files changed, 111 insertions(+), 137 deletions(-)
---
diff --git a/libxml++/parsers/domparser.h b/libxml++/parsers/domparser.h
index f90aa7a..cc5bc91 100644
--- a/libxml++/parsers/domparser.h
+++ b/libxml++/parsers/domparser.h
@@ -63,7 +63,7 @@ public:
    * @throws xmlpp::parse_error
    * @throws xmlpp::validity_error
    */
-  void parse_memory_raw(const unsigned char* contents, size_type bytes_count);
+  void parse_memory_raw(const unsigned char* contents, size_type bytes_count) override;
 
   /** Parse an XML document from a stream.
    * If the parser already contains a document, that document and all its nodes
diff --git a/libxml++/parsers/parser.cc b/libxml++/parsers/parser.cc
index bd3865c..70d9eb8 100644
--- a/libxml++/parsers/parser.cc
+++ b/libxml++/parsers/parser.cc
@@ -4,150 +4,106 @@
  * included with libxml++ as the file COPYING.
  */
 
-// Include glibmm/threads.h first. It must be the first file to include glib.h,
-// because it temporarily undefines G_DISABLE_DEPRECATED while it includes glib.h.
-#include <glibmm/threads.h> // For Glib::Threads::Mutex. Needed until the next API/ABI break.
-
 #include "libxml++/exceptions/wrapped_exception.h"
 #include "libxml++/parsers/parser.h"
 
 #include <libxml/parser.h>
 
-#include <memory> //For unique_ptr.
-#include <map>
-
-//TODO: See several TODOs in parser.h for changes at the next API/ABI break.
-
-namespace // anonymous
+namespace xmlpp
 {
-// These are new data members that can't be added to xmlpp::Parser now,
-// because it would break ABI.
-struct ExtraParserData
+
+struct Parser::Impl
 {
-  // Strange default values for throw_*_messages chosen for backward compatibility.
-  ExtraParserData()
-  : throw_parser_messages_(false), throw_validity_messages_(true),
+  Impl()
+  :
+  throw_messages_(true), validate_(false), substitute_entities_(false),
   include_default_attributes_(false), set_options_(0), clear_options_(0)
   {}
 
+  // Built gradually - used in an exception at the end of parsing.
   Glib::ustring parser_error_;
   Glib::ustring parser_warning_;
-  bool throw_parser_messages_;
-  bool throw_validity_messages_;
+  Glib::ustring validate_error_;
+  Glib::ustring validate_warning_;
+
+  bool throw_messages_;
+  bool validate_;
+  bool substitute_entities_;
   bool include_default_attributes_;
   int set_options_;
   int clear_options_;
 };
 
-std::map<const xmlpp::Parser*, ExtraParserData> extra_parser_data;
-// Different Parser instances may run in different threads.
-// Accesses to extra_parser_data must be thread-safe.
-Glib::Threads::Mutex extra_parser_data_mutex;
-
-void on_parser_error(const xmlpp::Parser* parser, const Glib::ustring& message)
-{
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  //Throw an exception later when the whole message has been received:
-  extra_parser_data[parser].parser_error_ += message;
-}
-
-void on_parser_warning(const xmlpp::Parser* parser, const Glib::ustring& message)
-{
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  //Throw an exception later when the whole message has been received:
-  extra_parser_data[parser].parser_warning_ += message;
-}
-} // anonymous
-
-namespace xmlpp {
-
 Parser::Parser()
-: context_(nullptr), exception_(nullptr), validate_(false), substitute_entities_(false) //See doxygen 
comment on set_substiute_entities().
+: context_(nullptr), exception_(nullptr), pimpl_(new Impl)
 {
-
 }
 
 Parser::~Parser()
 {
   release_underlying();
   delete exception_;
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  extra_parser_data.erase(this);
 }
 
 void Parser::set_validate(bool val)
 {
-  validate_ = val;
+  pimpl_->validate_ = val;
 }
 
 bool Parser::get_validate() const
 {
-  return validate_;
+  return pimpl_->validate_;
 }
 
 void Parser::set_substitute_entities(bool val)
 {
-  substitute_entities_ = val;
+  pimpl_->substitute_entities_ = val;
 }
 
 bool Parser::get_substitute_entities() const
 {
-  return substitute_entities_;
+  return pimpl_->substitute_entities_;
 }
 
 void Parser::set_throw_messages(bool val)
 {
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  extra_parser_data[this].throw_parser_messages_ = val;
-  extra_parser_data[this].throw_validity_messages_ = val;
+  pimpl_->throw_messages_ = val;
 }
 
 bool Parser::get_throw_messages() const
 {
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  return extra_parser_data[this].throw_parser_messages_;
+  return pimpl_->throw_messages_;
 }
 
 void Parser::set_include_default_attributes(bool val)
 {
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  extra_parser_data[this].include_default_attributes_ = val;
+  pimpl_->include_default_attributes_ = val;
 }
 
 bool Parser::get_include_default_attributes()
 {
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  return extra_parser_data[this].include_default_attributes_;
+  return pimpl_->include_default_attributes_;
 }
 
 void Parser::set_parser_options(int set_options, int clear_options)
 {
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  extra_parser_data[this].set_options_ = set_options;
-  extra_parser_data[this].clear_options_ = clear_options;
+  pimpl_->set_options_ = set_options;
+  pimpl_->clear_options_ = clear_options;
 }
 
 void Parser::get_parser_options(int& set_options, int& clear_options)
 {
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  set_options = extra_parser_data[this].set_options_;
-  clear_options = extra_parser_data[this].clear_options_;
+  set_options = pimpl_->set_options_;
+  clear_options = pimpl_->clear_options_;
 }
 
 void Parser::initialize_context()
 {
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-
   //Clear these temporary buffers:
-  extra_parser_data[this].parser_error_.erase();
-  extra_parser_data[this].parser_warning_.erase();
-  validate_error_.erase();
-  validate_warning_.erase();
-
-  // Take a copy of the extra data, so we don't have to access
-  // the extra_parser_data map more than necessary.
-  const auto extra_parser_data_this = extra_parser_data[this];
-  lock.release();
+  pimpl_->parser_error_.erase();
+  pimpl_->parser_warning_.erase();
+  pimpl_->validate_error_.erase();
+  pimpl_->validate_warning_.erase();
 
   //Disactivate any non-standards-compliant libxml1 features.
   //These are disactivated by default, but if we don't deactivate them for each context
@@ -157,28 +113,28 @@ void Parser::initialize_context()
 
   //Turn on/off validation, entity substitution and default attribute inclusion.
   int options = context_->options;
-  if (validate_)
+  if (pimpl_->validate_)
     options |= XML_PARSE_DTDVALID;
   else
     options &= ~XML_PARSE_DTDVALID;
 
-  if (substitute_entities_)
+  if (pimpl_->substitute_entities_)
     options |= XML_PARSE_NOENT;
   else
     options &= ~XML_PARSE_NOENT;
 
-  if (extra_parser_data_this.include_default_attributes_)
+  if (pimpl_->include_default_attributes_)
     options |= XML_PARSE_DTDATTR;
   else
     options &= ~XML_PARSE_DTDATTR;
 
   //Turn on/off any parser options.
-  options |= extra_parser_data_this.set_options_;
-  options &= ~extra_parser_data_this.clear_options_;
+  options |= pimpl_->set_options_;
+  options &= ~pimpl_->clear_options_;
 
   xmlCtxtUseOptions(context_, options);
 
-  if (context_->sax && extra_parser_data_this.throw_parser_messages_)
+  if (context_->sax && pimpl_->throw_messages_)
   {
     //Tell the parser context about the callbacks.
     context_->sax->fatalError = &callback_parser_error;
@@ -186,7 +142,7 @@ void Parser::initialize_context()
     context_->sax->warning = &callback_parser_warning;
   }
 
-  if (extra_parser_data_this.throw_validity_messages_)
+  if (pimpl_->throw_messages_)
   {
     //Tell the validity context about the callbacks:
     //(These are only called if validation is on - see above)
@@ -194,7 +150,7 @@ void Parser::initialize_context()
     context_->vctxt.warning = &callback_validity_warning;
   }
 
-  //Allow the callback_validity_*() methods to retrieve the C++ instance:
+  //Allow callback_error_or_warning() to retrieve the C++ instance:
   context_->_private = this;
 }
 
@@ -214,51 +170,61 @@ void Parser::release_underlying()
   }
 }
 
+void Parser::on_parser_error(const Glib::ustring& message)
+{
+  //Throw an exception later when the whole message has been received:
+  pimpl_->parser_error_ += message;
+}
+
+void Parser::on_parser_warning(const Glib::ustring& message)
+{
+  //Throw an exception later when the whole message has been received:
+  pimpl_->parser_warning_ += message;
+}
 void Parser::on_validity_error(const Glib::ustring& message)
 {
   //Throw an exception later when the whole message has been received:
-  validate_error_ += message;
+  pimpl_->validate_error_ += message;
 }
 
 void Parser::on_validity_warning(const Glib::ustring& message)
 {
   //Throw an exception later when the whole message has been received:
-  validate_warning_ += message;
+  pimpl_->validate_warning_ += message;
 }
 
-void Parser::check_for_validity_messages() // Also checks parser messages
+void Parser::check_for_error_and_warning_messages()
 {
   Glib::ustring msg(exception_ ? exception_->what() : "");
   bool parser_msg = false;
   bool validity_msg = false;
 
-  Glib::Threads::Mutex::Lock lock(extra_parser_data_mutex);
-  if (!extra_parser_data[this].parser_error_.empty())
+  if (!pimpl_->parser_error_.empty())
   {
     parser_msg = true;
-    msg += "\nParser error:\n" + extra_parser_data[this].parser_error_;
-    extra_parser_data[this].parser_error_.erase();
+    msg += "\nParser error:\n" + pimpl_->parser_error_;
+    pimpl_->parser_error_.erase();
   }
 
-  if (!extra_parser_data[this].parser_warning_.empty())
+  if (!pimpl_->parser_warning_.empty())
   {
     parser_msg = true;
-    msg += "\nParser warning:\n" + extra_parser_data[this].parser_warning_;
-    extra_parser_data[this].parser_warning_.erase();
+    msg += "\nParser warning:\n" + pimpl_->parser_warning_;
+    pimpl_->parser_warning_.erase();
   }
 
-  if (!validate_error_.empty())
+  if (!pimpl_->validate_error_.empty())
   {
     validity_msg = true;
-    msg += "\nValidity error:\n" + validate_error_;
-    validate_error_.erase();
+    msg += "\nValidity error:\n" + pimpl_->validate_error_;
+    pimpl_->validate_error_.erase();
   }
 
-  if (!validate_warning_.empty())
+  if (!pimpl_->validate_warning_.empty())
   {
     validity_msg = true;
-    msg += "\nValidity warning:\n" + validate_warning_;
-    validate_warning_.erase();
+    msg += "\nValidity warning:\n" + pimpl_->validate_warning_;
+    pimpl_->validate_warning_.erase();
   }
 
   if (parser_msg || validity_msg)
@@ -271,6 +237,7 @@ void Parser::check_for_validity_messages() // Also checks parser messages
   }
 }
   
+//static
 void Parser::callback_parser_error(void* ctx, const char* msg, ...)
 {
   va_list var_args;
@@ -279,6 +246,7 @@ void Parser::callback_parser_error(void* ctx, const char* msg, ...)
   va_end(var_args);
 }
 
+//static
 void Parser::callback_parser_warning(void* ctx, const char* msg, ...)
 {
   va_list var_args;
@@ -287,6 +255,7 @@ void Parser::callback_parser_warning(void* ctx, const char* msg, ...)
   va_end(var_args);
 }
 
+//static
 void Parser::callback_validity_error(void* ctx, const char* msg, ...)
 {
   va_list var_args;
@@ -295,6 +264,7 @@ void Parser::callback_validity_error(void* ctx, const char* msg, ...)
   va_end(var_args);
 }
 
+//static
 void Parser::callback_validity_warning(void* ctx, const char* msg, ...)
 {
   va_list var_args;
@@ -303,6 +273,7 @@ void Parser::callback_validity_warning(void* ctx, const char* msg, ...)
   va_end(var_args);
 }
 
+//static
 void Parser::callback_error_or_warning(MsgType msg_type, void* ctx,
                                        const char* msg, va_list var_args)
 {
@@ -334,10 +305,10 @@ void Parser::callback_error_or_warning(MsgType msg_type, void* ctx,
         switch (msg_type)
         {
           case MsgParserError:
-            on_parser_error(parser, ubuff);
+            parser->on_parser_error(ubuff);
             break;
           case MsgParserWarning:
-            on_parser_warning(parser, ubuff);
+            parser->on_parser_warning(ubuff);
             break;
           case MsgValidityError:
             parser->on_validity_error(ubuff);
@@ -372,7 +343,7 @@ void Parser::handleException(const exception& e)
 
 void Parser::check_for_exception()
 {
-  check_for_validity_messages();
+  check_for_error_and_warning_messages();
   
   if(exception_)
   {
diff --git a/libxml++/parsers/parser.h b/libxml++/parsers/parser.h
index 96034af..21f8a35 100644
--- a/libxml++/parsers/parser.h
+++ b/libxml++/parsers/parser.h
@@ -17,6 +17,7 @@
 
 #include <istream>
 #include <cstdarg> //For va_list.
+#include <memory> // std::unique_ptr
 
 #ifndef DOXYGEN_SHOULD_SKIP_THIS
 extern "C" {
@@ -28,6 +29,7 @@ namespace xmlpp {
 
 /** XML parser.
  *
+ * Abstract base class for DOM parser and SAX parser.
  */
 class Parser : NonCopyable
 {
@@ -37,39 +39,45 @@ public:
 
   typedef unsigned int size_type;
 
-  //TODO: Remove virtuals when we can break ABI.
-
   /** By default, the parser will not validate the XML file.
    * @param val Whether the document should be validated.
    */
-  virtual void set_validate(bool val = true);
+  void set_validate(bool val = true);
 
   /** See set_validate().
    * @returns Whether the parser will validate the XML file.
    */
-  virtual bool get_validate() const;
+  bool get_validate() const;
 
   /** Set whether the parser will automatically substitute entity references with the text of the entities' 
definitions.
    * For instance, this affects the text returned by ContentNode::get_content().
    * By default, the parser will not substitute entities, so that you do not lose the entity reference 
information.
    * @param val Whether entities will be substitued.
    */
-  virtual void set_substitute_entities(bool val = true);
+  void set_substitute_entities(bool val = true);
 
   /** See set_substitute_entities().
    * @returns Whether entities will be substituted during parsing.
    */
-  virtual bool get_substitute_entities() const;
+  bool get_substitute_entities() const;
 
   /** Set whether the parser will collect and throw error and warning messages.
+   *
    * If messages are collected, they are included in an exception thrown at the
-   * end of parsing. If the messages are not collected, they are written on
-   * stderr. The messages written on stderr are slightly different, and may
-   * be preferred in a program started from the command-line.
+   * end of parsing.
+   *
+   * - DOM parser
+   *
+   *   If the messages are not collected, they are written on stderr.
+   *   The messages written on stderr are slightly different, and may be
+   *   preferred in a program started from the command-line. The default, if
+   *   set_throw_messages() is not called, is to collect and throw messages.
+   *
+   * - SAX parser
    *
-   * The default, if set_throw_messages() is not called, is to collect and throw
-   * only messages from validation. Other messages are written to stderr.
-   * This is for backward compatibility, and may change in the future.
+   *   If the messages are not collected, the error handling on_*() methods in
+   *   the user's SAX parser subclass are called. This is the default, if
+   *   set_throw_messages() is not called.
    *
    * @newin{2,36}
    *
@@ -82,7 +90,6 @@ public:
    * @newin{2,36}
    *
    * @returns Whether messages will be collected and thrown in an exception.
-   *          The default with only validation messages thrown is returned as false.
    */
   bool get_throw_messages() const;
 
@@ -136,7 +143,12 @@ public:
    */
   virtual void parse_file(const Glib::ustring& filename) = 0;
 
-  //TODO: In a future ABI-break, add a virtual void parse_memory_raw(const unsigned char* contents, 
size_type bytes_count);
+  /** Parse an XML document from raw memory.
+   * @throw exception
+   * @param contents The XML document as an array of bytes.
+   * @param bytes_count The number of bytes in the @a contents array.
+   */
+  virtual void parse_memory_raw(const unsigned char* contents, size_type bytes_count) = 0;
   
   /** Parse an XML document from a string.
    * @throw exception
@@ -156,19 +168,16 @@ protected:
   virtual void initialize_context();
   virtual void release_underlying();
 
-  //TODO: In a future ABI-break, add these virtual functions.
-  //virtual void on_parser_error(const Glib::ustring& message);
-  //virtual void on_parser_warning(const Glib::ustring& message);
+  virtual void on_parser_error(const Glib::ustring& message);
+  virtual void on_parser_warning(const Glib::ustring& message);
   virtual void on_validity_error(const Glib::ustring& message);
   virtual void on_validity_warning(const Glib::ustring& message);
 
   virtual void handleException(const exception& e);
   virtual void check_for_exception();
 
-  //TODO: In a future API/ABI-break, change the name of this function to
-  // something more appropriate, such as check_for_error_and_warning_messages.
-  virtual void check_for_validity_messages();
-  
+  virtual void check_for_error_and_warning_messages();
+
   static void callback_parser_error(void* ctx, const char* msg, ...);
   static void callback_parser_warning(void* ctx, const char* msg, ...);
   static void callback_validity_error(void* ctx, const char* msg, ...);
@@ -187,19 +196,10 @@ protected:
 
   _xmlParserCtxt* context_;
   exception* exception_;
-  //TODO: In a future ABI-break, add these members.
-  //bool throw_messages_;
-  //Glib::ustring parser_error_;
-  //Glib::ustring parser_warning_;
-  Glib::ustring validate_error_;
-  Glib::ustring validate_warning_; //Built gradually - used in an exception at the end of parsing.
-
-  bool validate_;
-  bool substitute_entities_;
-  //TODO: In a future ABI-break, add these members.
-  //bool include_default_attributes_;
-  //int set_options_;
-  //int clear_options_;
+
+private:
+  struct Impl;
+  std::unique_ptr<Impl> pimpl_;
 };
 
 /** Equivalent to Parser::parse_stream().
diff --git a/libxml++/parsers/saxparser.cc b/libxml++/parsers/saxparser.cc
index 17531b2..ea81762 100644
--- a/libxml++/parsers/saxparser.cc
+++ b/libxml++/parsers/saxparser.cc
@@ -77,6 +77,9 @@ SaxParser::SaxParser(bool use_get_entity)
     0,  // serror
   };
   *sax_handler_ = temp;
+
+  // The default action is to call on_warning(), on_error(), on_fatal_error().
+  set_throw_messages(false);
 }
 
 SaxParser::~SaxParser()
diff --git a/libxml++/parsers/saxparser.h b/libxml++/parsers/saxparser.h
index 3ed8e1e..846c959 100644
--- a/libxml++/parsers/saxparser.h
+++ b/libxml++/parsers/saxparser.h
@@ -103,7 +103,7 @@ public:
    * @throws xmlpp::parse_error
    * @throws xmlpp::validity_error
    */
-  void parse_memory_raw(const unsigned char* contents, size_type bytes_count);
+  void parse_memory_raw(const unsigned char* contents, size_type bytes_count) override;
 
   /** Parse an XML document from a stream.
    * @param in The stream.