Re: [lmi] xmlwrapp '-Wconversion' warnings

[Vadim Zeitlin]

On Mon, 1 Apr 2019 16:14:38 +0000 Greg Chicares <address@hidden> wrote:

GC> On 2019-03-27 10:23, Vadim Zeitlin wrote:
GC> > On Wed, 27 Mar 2019 00:40:15 +0000 Greg Chicares <address@hidden> wrote:
GC> [...]
GC> > GC> e.clear();
GC> > GC> // XMLWRAPP !! Someday, this might be rewritten thus:
GC> > GC> //   e.set_content(value_cast<std::string>(t).c_str());
GC> > GC> // but for now that doesn't work with embedded ampersands.
GC> > GC> 
e.push_back(xml::node(xml::node::text(value_cast<std::string>(t).c_str())));
GC> > 
GC> >  Hmm, hasn't this been addressed by the addition of set_text_content() 
back
GC> > in xmlwrapp 0.7.0? I've just applied the following patch:
GC> > 
GC> > ---------------------------------- >8 
--------------------------------------
GC> > diff --git a/xml_serialize.hpp b/xml_serialize.hpp
GC> > index 5c6e90156..cef39daff 100644
GC> > --- a/xml_serialize.hpp
GC> > +++ b/xml_serialize.hpp
GC> > @@ -60,11 +60,7 @@ struct xml_io
GC> > 
GC> >      static void to_xml(xml::element& e, T const& t)
GC> >      {
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())));
GC> > +        e.set_text_content(value_cast<std::string>(t).c_str());
GC> >      }
GC> > 
GC> >      static void from_xml(xml::element const& e, T& t)
GC> > ---------------------------------- >8 
--------------------------------------
GC> 
GC> This induces a superficial regression in generated product files, e.g.:
GC> 
GC> --- data/sample.rounding        2019-01-23 19:22:18.930313781 +0000
GC> +++ /opt/lmi/data/sample.rounding       2019-04-01 15:19:50.547602533 +0000
GC> @@ -5,106 +5,106 @@
GC>    <RoundCoiCharge>
GC>      <decimals>2</decimals>
GC>      <style>To nearest</style>
GC> -    <gloss></gloss>
GC> +    <gloss/>
GC>    </RoundCoiCharge>
GC> ...
GC> 
GC> "<gloss></gloss>" and "<gloss/>" are of course equivalent but not identical.

 Yes, sorry, I hadn't realized this patch would result in this change in
output even though it does seem clear in hindsight: previously we
explicitly created an empty text node as child of the element "e" while now
we set contents of this element itself to the given text value. So to
preserve exactly the same output as before we really need to do something
like

        xml::node n;
        n.set_text_content(value_cast<std::string>(t).c_str());
        e.push_back(n);

which is not really better than what we did before.

GC> I've always preferred the compact form because of terseness,

 Me too, even though I know that the canonical form is the full one.

GC> Is it definitely your intention in xmlwrapp that set_text_content() prefer
GC> the compact form despite its being noncanonical?

 As usual, xmlwrapp doesn't do anything non-trivial here, all it does it
call libxml2 functions and writing XML out is implemented entirely in the
latter library and it happens to prefer the compact form by default as
well. However it does provide XML_SAVE_NO_EMPTY flag which can be used to
avoid creating the compact empty tags. Using it is not completely trivial
as flags can only be specified using lower-level API and not the high-level
functions we currently use, but it should definitely be possible, so I
could look into doing this if you think it would be worth adding the
possibility to ensure that we never get any non-canonical empty tags in the
XML output.

GC> I'm not sure I have an opinion either way; but this causes regressions
GC> in our proprietary product-files repository, and I don't want to push a
GC> commit to adapt its (version-controlled) generated files to this change
GC> until I'm sure you won't want to change xmlwrapp to make
GC> set_text_content()'s output canonical.

 I'm quite sure that we're not going to change how set_text_content()
works, i.e. we won't create a child text node in it just to avoid creating
a compact empty tag on output. However we could add support for a flag
specifying that empty nodes should be represented in the canonical form on
output to xml::document::save_to_{file,string}() functions.

GC> > xml_serialize_test still passes (and, to be complete, it does not
GC> > pass, as expected and indicated by the comment, if I use just
GC> > set_content() above instead)
GC> 
GC> lmi's 'xml_serialize_test.cpp' contains:
GC>   std::string      const s0("string with ampersand & embedded spaces");
GC> but AFAICS all tests pass both with and without the patch above.
GC> What am I missing?

 Maybe nothing and I just didn't explain myself correctly: the test does
pass both with the original version and the new one, however it does not
pass if I use the commented out version shown in the old TODO comment, i.e.

        e.set_content(value_cast<std::string>(t).c_str());

does not escape the ampersands, while set_text_content() does.

 Please let me know if you still have questions about this test and/or if
you'd like me to look more into XML_SAVE_NO_EMPTY support.

 Thanks,
VZ