[librsvg: 6/7] XmlState: don't take() the document_builder in the end; just consume everything

[Marge Bot <marge-bot src gnome org>]

commit cb9201cdb67e62efacee2282b251d544c0000bc0
Author: Federico Mena Quintero <federico gnome org>
Date:   Fri Sep 23 12:21:10 2022 -0500

    XmlState: don't take() the document_builder in the end; just consume everything
    
    DocumentBuilder.build() consumes its self argument, which makes sense:
    it turns the builder into a document, and that's that.
    
    However, XmlStateInner had an Option<DocumentBuilder> field just so
    that the option could be take()n in the end and then the builder
    could be consumed by .build().
    
    Using an Option<DocumentBuilder> is inconvenient because the rest of
    the code must then do inner.document_builder.as_mut().unwrap()
    everywhere.  We could have used a helper function to do that, but we
    can remove that construct altogether.
    
    One thing to note is that since we are consuming the final XmlState
    and just pick out its inner field in build_document(), we can no
    longer impl Drop for XmlState to free the hash table of libxml2
    entities.  So, we inline the code to free the hash in
    build_document() instead.
    
    Part-of: <https://gitlab.gnome.org/GNOME/librsvg/-/merge_requests/752>

 src/xml/mod.rs | 58 +++++++++++++++++++++++++---------------------------------
 1 file changed, 25 insertions(+), 33 deletions(-)
---
diff --git a/src/xml/mod.rs b/src/xml/mod.rs
index deae17900..32652674b 100644
--- a/src/xml/mod.rs
+++ b/src/xml/mod.rs
@@ -104,11 +104,18 @@ macro_rules! xinclude_name {
 /// trait objects. Normally the context refers to a `NodeCreationContext` implementation which is
 /// what creates normal graphical elements.
 struct XmlStateInner {
-    document_builder: Option<DocumentBuilder>,
+    document_builder: DocumentBuilder,
     num_loaded_elements: usize,
     context_stack: Vec<Context>,
     current_node: Option<Node>,
 
+    // Note that neither XmlStateInner nor Xmlstate implement Drop.
+    //
+    // An XmlState is finally consumed in XmlState::build_document(), and that
+    // function is responsible for freeing all the XmlEntityPtr from this field.
+    //
+    // (The structs cannot impl Drop because build_document()
+    // destructures and consumes them at the same time.)
     entities: HashMap<String, XmlEntityPtr>,
 }
 
@@ -146,7 +153,7 @@ impl XmlState {
     ) -> XmlState {
         XmlState {
             inner: RefCell::new(XmlStateInner {
-                document_builder: Some(document_builder),
+                document_builder,
                 num_loaded_elements: 0,
                 context_stack: vec![Context::Start],
                 current_node: None,
@@ -291,11 +298,7 @@ impl XmlState {
                     if let Ok(stylesheet) =
                         Stylesheet::from_href(&aurl, Origin::Author, self.session.clone())
                     {
-                        inner
-                            .document_builder
-                            .as_mut()
-                            .unwrap()
-                            .append_stylesheet(stylesheet);
+                        inner.document_builder.append_stylesheet(stylesheet);
                     } else {
                         // FIXME: https://www.w3.org/TR/xml-stylesheet/ does not seem to specify
                         // what to do if the stylesheet cannot be loaded, so here we ignore the error.
@@ -355,11 +358,7 @@ impl XmlState {
             let mut inner = self.inner.borrow_mut();
 
             let parent = inner.current_node.clone();
-            let node = inner
-                .document_builder
-                .as_mut()
-                .unwrap()
-                .append_element(name, attrs, parent);
+            let node = inner.document_builder.append_element(name, attrs, parent);
             inner.current_node = Some(node);
 
             if name.expanded() == expanded_name!(svg "style") {
@@ -380,11 +379,7 @@ impl XmlState {
         let mut inner = self.inner.borrow_mut();
 
         let mut parent = inner.current_node.clone().unwrap();
-        inner
-            .document_builder
-            .as_mut()
-            .unwrap()
-            .append_characters(text, &mut parent);
+        inner.document_builder.append_characters(text, &mut parent);
     }
 
     fn style_end_element(&self) {
@@ -409,15 +404,13 @@ impl XmlState {
                 })
                 .collect::<String>();
 
-            let builder = inner.document_builder.as_mut().unwrap();
-
             if let Ok(stylesheet) = Stylesheet::from_data(
                 &stylesheet_text,
                 &self.load_options.url_resolver,
                 Origin::Author,
                 self.session.clone(),
             ) {
-                builder.append_stylesheet(stylesheet);
+                inner.document_builder.append_stylesheet(stylesheet);
             } else {
                 rsvg_log!(self.session, "invalid inline stylesheet");
             }
@@ -624,24 +617,23 @@ impl XmlState {
     ) -> Result<Document, LoadingError> {
         self.parse_from_stream(stream, cancellable)?;
 
-        self.inner
-            .borrow_mut()
-            .document_builder
-            .take()
-            .unwrap()
-            .build()
-    }
-}
+        // consume self, then consume inner, then consume document_builder by calling .build()
 
-impl Drop for XmlState {
-    fn drop(&mut self) {
-        unsafe {
-            let mut inner = self.inner.borrow_mut();
+        let XmlState { inner, .. } = self;
+        let mut inner = inner.into_inner();
 
-            for (_key, entity) in inner.entities.drain() {
+        // Free the hash of XmlEntityPtr.  We cannot do this in Drop because we will
+        // consume inner by destructuring it after the for() loop.
+        for (_key, entity) in inner.entities.drain() {
+            unsafe {
                 xmlFreeNode(entity);
             }
         }
+
+        let XmlStateInner {
+            document_builder, ..
+        } = inner;
+        document_builder.build()
     }
 }