Re: [lmi] Requesting Evgeniy's review

[Evgeniy Tarassov]

On 11/1/06, Greg Chicares <address@hidden> wrote:
> I'm trying to refactor all xml-library code, ideally so that
> all calls to libxml++ occur in 'xml_lmi.?pp'. This may or
> may not give us the option of substituting different xml
> libraries, which may or may not be a good goal; but it is
> educationally valuable to me, and I feel that the effort is
> yielding clearer code. And safer code, too, because it's
> easier to make sure all exceptions are trapped and all error
> codes are checked. And above all it makes the transition
> from xmlwrapp to libxml++ easier for me. Anyway, that's not
> what I wanted to ask you about....

I'm terribly sorry -- this message got lost in my mailbox!
IMHO the modifications are perfectly valid and looks much nicer.

Code snippet from ledger_xml_io.cpp
| #if 1
|         xml_lmi::Element const& root_node = parser.root_node("columns");
|
|         xml_lmi::NodeContainer const columns =
root_node.get_children("column");
|         for
|             (xml_lmi::NodeContainer::const_iterator it = columns.begin()
|             ,end = columns.end()
|             ;it != end
|             ;++it
|             )
| #else // not 1
| // EVGENIY This section is for your review.
|         xml_lmi::Element const& root_node = parser.root_node("columns");
|         xml_lmi::ElementContainer const columns
|             (xml_lmi::child_elements(root_node, "column")
|             );
|         typedef xml_lmi::ElementContainer::const_iterator eci;
|         for(eci it = columns.begin(); it != columns.end(); ++it)
| #endif // not 1
|             {
|             xml_lmi::Element const* column_element
|                 = dynamic_cast<xml_lmi::Element const*>(*it);

The only comment could be that the line above does not need the
dynamic_cast since ElementContainer contains non-null pointers to
xmlpp::Element objects.
And directly below that the code snippet should be removed too:
|             // a 'column' node is not an element node, skip it
|             if(!column_element)
|                 {
|                 continue;
|                 }


Below another #if 1/0 block from ledger_xml_io.cpp:
| #if 1
|             xml_lmi::NodeContainer const formats =
(*it)->get_children("format");
|             // skip nodes without format information
|             if(formats.empty())
|                 {
|                 continue;
|                 }
|
|             xml_lmi::Element const* format_element
|                 = dynamic_cast<xml_lmi::Element const*>(*formats.begin());
|             // a 'column/format' node is not an element node, skip it
|             if(!format_element)
|                 {
|                 continue;
|                 }
| #else // not 1
| // EVGENIY This section is for your review.
|             xml_lmi::ElementContainer const formats
|                 (xml_lmi::child_elements(**it, "format")
|                 );
|
|             // skip nodes without format information
|             if(formats.empty())
|                 {
|                 continue;
|                 }
|
|             xml_lmi::Element const* format_element = formats[0];
|             // EVGENIY What does the following statement actually do?
|             // I don't understand how the if-condition can ever be true.
|             //
|             // a 'column/format' node is not an element node, skip it
|             if(!format_element)
|                 {
|                 continue;
|                 }
| #endif // not 1

Looks perfect too. And about the question:

|             // EVGENIY What does the following statement actually do?
|             // I don't understand how the if-condition can ever be true.
|             //
|             // a 'column/format' node is not an element node, skip it
|             if(!format_element)

I have put another if() only so that if one day the xmlpp library
returns attribute nodes in xmlpp::Node::get_children too, the program
does not crash.

The problem is that xmlpp documentation to xmlpp::get_children is
vague and it is not clear weither the function returns text nodes,
attribute nodes and element nodes in that list, or just element nodes.
When the name argument to that function is not empty, will it return
element-nodes and attribute-nodes or only element-nodes?. Looking at
the code one could tell that its going to be xmlpp::Elements only, but
there is always a possibility that it could change in the future.
Now your wrapper xml_lmi::child_elements does the dynamic_cast to
xmlpp::Element and makes sure attribute nodes are not included in the
returned node-list. So this if block could be removed.

Do you want me to remove the #if 0/1 blocks myself (and apply your
changes, and test it) in the xml-gnome-branch?

> The last remaining use of this type:
>     typedef std::list<xmlpp::Node*> NodeContainer;
> (outside of 'xml_lmi.?pp') is in 'ledger_xml_io.cpp'. I made
> tentative modifications there, in '#if 0' blocks so that I
> wouldn't break anything. Could I ask you to review it over
> the next day or two and let me know whether I've done it
> correctly? It's different from other uses of 'NodeContainer'
> because you are calling
>     xmlpp::Node::get_children(argument)
> with an actual argument, whereas elsewhere the argument is
> just the default empty string. I've added such an argument
> to this function
>     xml_lmi::ElementContainer xml_lmi::child_elements
>         (xml_lmi::Element const& parent
>         ,std::string const& name
>         )
> but haven't studied 'double_formatter_t' well enough to be
> sure I've done this correctly.


--
ET