Re: [boost] [Boost Review] Property Tree Library

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2006-04-19 17:15:45

"Thorsten Ottosen" wrote:

> I have the great pleasure to announce that the formal boost-review of
> Marcin Kalicinski Property Tree Library
>

I vote weak yes to accept the library.

Personally, I would prefere to wait for rev6
until some issues get resolved. Given that
the review had started I think it is better
to accept the library as it is very likely to get
maintained and improved. Not accepting
it would mean 6-12 months delay and possibly
abandonment.

I did a review of rev5 few days ago
- the notes are pasted bellow if anyone
is interested.

/Pavel

-------------------------------------------------------------------

Hello Marcin,

I took look on the V5 and collected few notes.

Most important, the documentation still needs
quite a lot of work.

I read some noises about expanding the library to
be arbitrary tree and so on.
IME this is very hard to develop and of rather
low value for users.

Ptree 's main value is its simplicity for its intended task
and this should not be compromised.

/Pavel

_____________________________________________________
1. docs: it is rather unusual for me read code
   without syntax highlighting.

_____________________________________________________
2. The first mention about use of exceptions in docs
   (in debug_settings::load) should by hyperlink to details.

   I would welcome every code snippet to have such link,
   very visible to people who co copy + paste.

_____________________________________________________
3. The first mention of get_d() should explicitly say
   that "d" means default. If possible this word should
   be bolded.

_____________________________________________________
4. A curiousity: in docs you write:

 Type of the value extracted
 is determined by type of second parameter, so we can simply
 write get_d(...) instead of get_d<int>(...). */

 m_level = pt.get_d("debug.level", 0);

If I do not specify return type explicitly there
may be conversion made after function returns.

If I specify the type explicitly there may be
conversion of the default parameters when the
thread of execution enters the function.

There may be some interesting side-effects
with this. At the moment I do not see
much use for the distinction or some idiom
based on it but perhaps it may be worth
of some attention.

_____________________________________________________
5. docs, "Property tree as a container": it may
   be explicitly stated here (or linked from here)
   how validity of iterators changes after delete/update/etc.

_____________________________________________________
6. Possible feature:

"...there is an additional indexing data structure
inside property tree that allows fast searches..."

There may be a type trait that disables creating
of such structure, where the search complexity
would degrade gracefuly to O(N).

For example a huge structure that is processed
sequentially may not need it (and would be better
with more memory).

_____________________________________________________
7. The temptation to use std::lower_bound and similarly
   dangeroud std algorithm may be eliminated by defining
   unimplemented specialisations for ptree
   (in std:: namespace). This is allowed by standard.

_____________________________________________________
8. docs, "Synopsis":

"Instances of this class are property trees."

- the sentence may be better reworded, it sounds
somehow redundant to me.

------------

The mention of ptree, wptree, iptree and wiptree
should be hyperlinked - it is absolutely unclear
what they are about.

------------

Perhaps tables could be used as a tool to combat
the cryptic abbreviations. These tables would have
list these abbreviations and rationale for the name.

The docs hyperlinks may point to such tables
for quick overview.

>From the table one would be able to go deeper
into references.

---------------
The lines

class ptree_error;
class ptree_bad_path;
class ptree_bad_data;

should have comment that these classes
are actually exceptions. It is not that clear.

------------
The name "ptree_error" would be better
"ptree_error_base" to give immediate clue

-----------------
empty_ptree() should be create_empty_tree().
or make_empty_tree() (I prefere the first,
the second is somehow semistandard).

--------------
In template<class Ch, ....

the "Ch" should be spelled fully. Ch gives no clue.
Dtto the "Tr".

---------------
The parametrisation of ptree should go futher.
The basic_string<Ch> should be template
parameter. Some people may like (or be forced)
to use flex_string/AnsiString/CString/QString/
wxString/boost::fixed_string/etc
and the library should not lay obstacles to them.

Another possible template parameter is allocator.

---------------

stable_unique() operation may be added.
This would eliminate all duplicates while
keeping order of what is kept.

A generic implementation of stable_unique
can be seen on
http://uk.builder.com/programming/c/0,39029981,20271583,00.htm

--------------------
The part of docs named

// Ptree-specific operations

should have more comments. It is almost unreadable blob
as it is now (no syntax highlighting, no hyperlinks
to actual code).

-------------------

get_own() etc: I suggest to add table of abbreviations
on the top of documentation, before anything else.
I feel rather stupid looking on "own" and guessing
what could it be (I try to feel as first time reader).

----------------
OTOH the locale is not needed to be described in such
detail in the documentation. Just someting
as --locale-- should be enough to get clue.

---------------
Instead of "class CharType"
a "typename CharType" feels better.

It is also somehow confusing what suddenly
new character type appears here and what
it could mean in the design.

There should be commen, possibly link to example.

_____________________________________________________
9. docs, "How to populate property tree"

The word "parsers" should not be used.
It brings feeling of Spirit or yacc.

A standard and not overloaded word is "reader"
or "reader/writer".

--------------

"It has just one data string associated..."

==>>

"It has just one data (typically string) associated..."

-----------
The sentence of what parts of XML (as multiple props)
are not supported should make it into
section of its own and this section should appear
in top table of contents.

-------------
Existence of file_parser_error is not shown in the
synopsis. There should be picture, class diagram
of all existing exceptions, possibly as clickable map.

-------------

The <xmlattr> discussion is now strangely splitted
among two sections (I am not able to distinguish if
they are or are not of the same level).

-----------

The sections may be numbered with x.y stype.

_____________________________________________________
10. docs, "INI parser":

The fillings as "the reason", "probably", "contrary",
"actually" should be omitted. The text will be read
by developer under time pressure and they won't appreaciate it.

_____________________________________________________
11. docs, "JSON parser": JSON could by
hyperlinked to external website.

_____________________________________________________
12. docs, "Command line parser" - existence
of Boost.Options library may be reminded in the begining
of this section.

_____________________________________________________
13. docs, "How to access data in property tree ":

"Property tree resembles (almost is) a standard container..."
is very ambiguous as there are many standard containers.

_____________________________________________________
14. docs: few examples may use wide strings als L"...".

_____________________________________________________
15. The headers may have

#if (defined _MSC_VER) && (_MSC_VER >= 1200)
#  pragma once
#endif

on the top to reduce a little bit compilation
time for VC and Intel.

_____________________________________________________
16. the file property_tree/detail/ptree_interface.hpp
    should be moved a folder down as it is the most
    important header, not an "detail".

_____________________________________________________
17. Idea of a feature: right now the ptree is intended
    as temporary structure - read from file and then
    transformed into user data.

    People may like to use ptree as primary structure,
    without need to define their own helper classes.
    For this it would be useful to be able to attach
    some data to nodes.

    I suggest to add yet another template parameter
    or type traits: associated datatype, defaulting to void.
    If they are not void then this datatype will exist
    next to each string. (Boost.Any may be useful example.)

_____________________________________________________
18. ptree_utils: the function widen/narrow are
    horribly inefficient. The "result" string
    should be reserve()d before characters are added.

    Since the only char types known to a man are
    singned char/unsigned char/char and wchar_t
    it would be better to provide specialisations
    for these. If someone wants something strange
    (string of doubles) he would need to write
    a meaningful specialisation for it.

    The narrow<wchar_t> is flawed.

    The function trim() is already available
    in Boost.String Algorithms.

_____________________________________________________
19. A nit: source files should end with newline
    (Standard says so). Some compilers may complain
    if they do not, e.g. ptree_interface.hpp.

_____________________________________________________
20. Namespaces: I would prefere not to have yet
    another sub-namespace in boost.

    The ptree should be moved down (via "using")
    so boost::ptree<...> would be valid.

_____________________________________________________
21. json_parser.hpp, create_escapes(): a switch
    should be used instead of the if-else chain.

-----------

The header is missing
#include <cctype>

_____________________________________________________
22. Boost.Serialization should be supported.
    It is not that absurd as it may look - the tree
    may be part of application state that is saved/sent
    over network.

_____________________________________________________
23. cmdline_parser.hpp: there may be support for
    parameters passed via WinMain() - i.e. as single
    string.

    The code that splits the string into argv tokens
    already exists in program_options library.

--------
I suspect the code:

 Ptree *child = local.put(text, Str());
 child->push_back(std::make_pair(Str(), Ptree(child->data())));

is not exception safe (when if the Ptree contructor throws,
who will clear the child). I have feeling an auto_ptr
may be useful here.

_____________________________________________________
24. The docs should show exception safety level for
    every member function. (e.g. as smart_ptrs have).

_____________________________________________________
25. I tried to compile the test with BCB (version 5.8),
    from BDS 2006.

    The problem is that Borland doesn't like the out-of-class
    member bodies that return any kind of iterator
    (begin/end/rbegin/rend/front/back/find/erase/push_front/push_back).

    I found a workaround: it is needed to fully specify
    the returned type, not just "iterator" but expanded
    definition:

So for example the

    template<class Ch, class Tr>
    typename basic_ptree<Ch, Tr>::iterator
        basic_ptree<Ch, Tr>::begin()
    {
        return m_impl->m_container.begin();
    }

would need to be changed to ugly:

    template<class Ch, class Tr>
#if BOOST_WORKAROUND(__BORLANDC__, BOOST_TESTED_AT(0x564))
    std::list<std::pair<std::basic_string<Ch>, basic_ptree<Ch, Tr> >
 >::iterator
#else
    typename basic_ptree<Ch, Tr>::iterator
#endif
        basic_ptree<Ch, Tr>::begin()
    {
        return m_impl->m_container.begin();
    }

Perhaps it is worth of have BCB support.
The compiler is quite shitty but their IDE
is so far the best C++ RAD environment on
the planet. Some people use it for this reason.

I can help with BCB porting. It is also
possible to download free BCB compiler
(version 5.5, more-less the same as the
5.8 compiler).

_____________________________________________________
26. For better visual appearance you may insert few <hr>
     into the documentation.
_____________________________________________________
EOF