RE: svn commit: r1765286 - in /subversion/trunk/subversion: include/svn_xml.h libsvn_subr/xml.c tests/libsvn_subr/xml-test.c

2016-10-17 Thread Bert Huijben


> -Original Message-
> From: i...@apache.org [mailto:i...@apache.org]
> Sent: maandag 17 oktober 2016 15:49
> To: commits@subversion.apache.org
> Subject: svn commit: r1765286 - in /subversion/trunk/subversion:
> include/svn_xml.h libsvn_subr/xml.c tests/libsvn_subr/xml-test.c
> 
> Author: ivan
> Date: Mon Oct 17 13:49:05 2016
> New Revision: 1765286
> 
> URL: http://svn.apache.org/viewvc?rev=1765286&view=rev
> Log:
> Implement standard lifetime semantics for svn_xml_parser_t: the object will be
> automatically freed on pool cleanup. But it still can be freed explicitly
> using svn_xml_free_parser(). It's the same behavior we already have for
> svn_sqlite__db_t and similar.

Are you planning a new use of this api?

It is currently only used by subversion/libsvn_wc/old-and-busted.c, and I don't 
think we should really spend time optimizing that specific usecase (reading pre 
1.4 working copies)

Bert



svn commit: r1765286 - in /subversion/trunk/subversion: include/svn_xml.h libsvn_subr/xml.c tests/libsvn_subr/xml-test.c

2016-10-17 Thread ivan
Author: ivan
Date: Mon Oct 17 13:49:05 2016
New Revision: 1765286

URL: http://svn.apache.org/viewvc?rev=1765286&view=rev
Log:
Implement standard lifetime semantics for svn_xml_parser_t: the object will be
automatically freed on pool cleanup. But it still can be freed explicitly
using svn_xml_free_parser(). It's the same behavior we already have for
svn_sqlite__db_t and similar.

* subversion/include/svn_xml.h
  (svn_xml_make_parser): Document existing and new behavior regarding
   svn_xml_parser_t lifetime.

* subversion/libsvn_subr/xml.c
  (parser_cleanup): New.
  (svn_xml_make_parser): Do not create SUBPOOL and allocate svn_parser_t from
   provided POOL. Register pool cleanup handler to free Expat parser.

* subversion/tests/libsvn_subr/xml-test.c
  (test_parser_free): New test.
  (test_funcs): Add test_parser_free to the test list.

Modified:
subversion/trunk/subversion/include/svn_xml.h
subversion/trunk/subversion/libsvn_subr/xml.c
subversion/trunk/subversion/tests/libsvn_subr/xml-test.c

Modified: subversion/trunk/subversion/include/svn_xml.h
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/include/svn_xml.h?rev=1765286&r1=1765285&r2=1765286&view=diff
==
--- subversion/trunk/subversion/include/svn_xml.h (original)
+++ subversion/trunk/subversion/include/svn_xml.h Mon Oct 17 13:49:05 2016
@@ -169,7 +169,15 @@ typedef void (*svn_xml_char_data)(void *
   apr_size_t len);
 
 
-/** Create a general Subversion XML parser */
+/** Create a general Subversion XML parser.
+ *
+ * The @c svn_xml_parser_t object itself will be allocated from @a pool,
+ * but some internal structures may be allocated out of pool.  Use
+ * svn_xml_free_parser() to free all memory used by the parser.
+ *
+ * Since Subversion 1.10 parser will be freed automatically on pool
+ * cleanup or by svn_xml_free_parser() call.
+ */
 svn_xml_parser_t *
 svn_xml_make_parser(void *baton,
 svn_xml_start_elem start_handler,

Modified: subversion/trunk/subversion/libsvn_subr/xml.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/xml.c?rev=1765286&r1=1765285&r2=1765286&view=diff
==
--- subversion/trunk/subversion/libsvn_subr/xml.c (original)
+++ subversion/trunk/subversion/libsvn_subr/xml.c Mon Oct 17 13:49:05 2016
@@ -364,6 +364,19 @@ static void expat_data_handler(void *use
 
 /*** Making a parser. ***/
 
+static apr_status_t parser_cleanup(void *data)
+{
+  svn_xml_parser_t *svn_parser = data;
+
+  /* Free Expat parser. */
+  if (svn_parser->parser)
+{
+  XML_ParserFree(svn_parser->parser);
+  svn_parser->parser = NULL;
+}
+  return APR_SUCCESS;
+}
+
 svn_xml_parser_t *
 svn_xml_make_parser(void *baton,
 svn_xml_start_elem start_handler,
@@ -372,8 +385,6 @@ svn_xml_make_parser(void *baton,
 apr_pool_t *pool)
 {
   svn_xml_parser_t *svn_parser;
-  apr_pool_t *subpool;
-
   XML_Parser parser = XML_ParserCreate(NULL);
 
   XML_SetElementHandler(parser,
@@ -382,22 +393,23 @@ svn_xml_make_parser(void *baton,
   XML_SetCharacterDataHandler(parser,
   data_handler ? expat_data_handler : NULL);
 
-  /* ### we probably don't want this pool; or at least we should pass it
- ### to the callbacks and clear it periodically.  */
-  subpool = svn_pool_create(pool);
-
-  svn_parser = apr_pcalloc(subpool, sizeof(*svn_parser));
+  svn_parser = apr_pcalloc(pool, sizeof(*svn_parser));
 
   svn_parser->parser = parser;
   svn_parser->start_handler = start_handler;
   svn_parser->end_handler = end_handler;
   svn_parser->data_handler = data_handler;
   svn_parser->baton = baton;
-  svn_parser->pool = subpool;
+  svn_parser->pool = pool;
 
   /* store our parser info as the UserData in the Expat parser */
   XML_SetUserData(parser, svn_parser);
 
+  /* Register pool cleanup handler to free Expat XML parser on cleanup,
+ if svn_xml_free_parser() was not called explicitly. */
+  apr_pool_cleanup_register(svn_parser->pool, svn_parser,
+parser_cleanup, apr_pool_cleanup_null);
+
   return svn_parser;
 }
 
@@ -406,11 +418,7 @@ svn_xml_make_parser(void *baton,
 void
 svn_xml_free_parser(svn_xml_parser_t *svn_parser)
 {
-  /* Free the expat parser */
-  XML_ParserFree(svn_parser->parser);
-
-  /* Free the subversion parser */
-  svn_pool_destroy(svn_parser->pool);
+  apr_pool_cleanup_run(svn_parser->pool, svn_parser, parser_cleanup);
 }
 
 

Modified: subversion/trunk/subversion/tests/libsvn_subr/xml-test.c
URL: 
http://svn.apache.org/viewvc/subversion/trunk/subversion/tests/libsvn_subr/xml-test.c?rev=1765286&r1=1765285&r2=1765286&view=diff
==
--- subversion/trunk/subversion/tests/libsvn_subr/xml-test.c (original)
+++ subversion/tr