Re: [lmi] xmlwrapp '-Wconversion' warnings

[Vadim Zeitlin]

On Wed, 27 Mar 2019 00:40:15 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2019-03-26 23:24, Vadim Zeitlin wrote:
GC> [...]
GC> >  AFAIR there were no other wishes for any changes in xmlwrapp, were there?
GC> This command
GC>   $vim -p `grep -rl 'XMLWRAPP !!' *`
GC> finds only three instance in three files.

 Sorry for completely forgetting about the existence of these markers, I
should have thought to check them myself, but thanks for doing it for me.

GC> /// XMLWRAPP !! xmlwrapp has no such ctor as
GC> ///   xml::tree_parser(std::istream&)
GC> 
GC> Unless the idea tickles your fancy, I'll remove that comment.
GC> The lack of such a ctor didn't prevent me from reading XML
GC> from an istream.

 At first, I thought it would be a no brainer to add such ctor, but, as
usual, things become more interesting when you look at them closer... The
reason we have tree_parser ctor from file name is that libxml2 provides
specific functions for parsing files, which have 2 advantages compared to
loading the file contents into memory and parsing it from there: first,
they implement automatic and transparent on the fly decompression and
second, they don't read the entire file into memory, but only do it
progressively, chunk by chunk.

 So now I'm less sure: on one hand, adding a wrapper ctor doing what the
existing lmi code in dom_parser ctor does, i.e. reading the entire stream
into memory and parsing it from there, would be trivial to do and would
still be useful (e.g. xmlwrapp own test suite could use this). OTOH it
would be even better to implement a full-featured ctor taking an input
stream and providing the same capabilities as the ctor taking the file
name, but this risks being more difficult. After a brief look at the code,
it looks like avoiding reading the entire stream into memory would be
relatively easy to implement, but I'm not sure about the decompression.
Of course, lmi probably doesn't care about either of these features, as
XML files it deals with are (relatively) small and not compressed, so I'm
not really sure about what to do here...

 Right now I think I'll try to add a "streaming" (i.e. reading data on
demand) ctor from a stream object but will fall back to just the dumb
version reading the entire stream into memory if this turns out to be too
complicated or, on the contrary, try to add support for decompression if I
do manage to do this quickly. Please let me know if you have any comments
about this plan.


GC> /// XMLWRAPP !! It is useful to distinguish elements from DOM
GC> /// nodes that are not elements; xmlwrapp doesn't make this
GC> /// distinction [...]
GC> 
GC> That sounds potentially useful to me, but maybe that's only
GC> because I don't understand XML and libxml2 well enough.

 It's true that xmlwrapp doesn't provide a separate xml::element type, e.g.
as a subclass of xml::node. I'm not sure if doing this would have been a
good idea or not, but in any case this seems to be very unlikely to happen,
so I don't see this changing.

GC> e.clear();
GC> // XMLWRAPP !! Someday, this might be rewritten thus:
GC> //   e.set_content(value_cast<std::string>(t).c_str());
GC> // but for now that doesn't work with embedded ampersands.
GC> e.push_back(xml::node(xml::node::text(value_cast<std::string>(t).c_str())));

 Hmm, hasn't this been addressed by the addition of set_text_content() back
in xmlwrapp 0.7.0? I've just applied the following patch:

---------------------------------- >8 --------------------------------------
diff --git a/xml_serialize.hpp b/xml_serialize.hpp
index 5c6e90156..cef39daff 100644
--- a/xml_serialize.hpp
+++ b/xml_serialize.hpp
@@ -60,11 +60,7 @@ struct xml_io

     static void to_xml(xml::element& e, T const& t)
     {
-        e.clear();
-        // XMLWRAPP !! Someday, this might be rewritten thus:
-        //   e.set_content(value_cast<std::string>(t).c_str());
-        // but for now that doesn't work with embedded ampersands.
-        
e.push_back(xml::node(xml::node::text(value_cast<std::string>(t).c_str())));
+        e.set_text_content(value_cast<std::string>(t).c_str());
     }

     static void from_xml(xml::element const& e, T& t)
---------------------------------- >8 --------------------------------------

xml_serialize_test still passes (and, to be complete, it does not pass, as
expected and indicated by the comment, if I use just set_content() above
instead), so I believe this marker can be fixed right now.

GC> None of these is really important for lmi, and I wouldn't
GC> hold up an xmlwrapp release for any of them. Let me know
GC> if you think any of these putative improvements are
GC> infeasible or just silly, so that I can remove them.

 I definitely think that something needs to be done about constructing
tree_parser from a stream. I don't believe anything can, or maybe even
should, be done about xml::element. And set_text_content() has already
been implemented, allowing me to complete the bingo by giving you all of
the 3 possible answers, one for each of the 3 remaining markers.

 Regards,
VZ