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
I did a review of rev5 few days ago
- the notes are pasted bellow if anyone
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.
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
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
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/
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
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"
"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
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
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
23. cmdline_parser.hpp: there may be support for
parameters passed via WinMain() - i.e. as single
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());
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
I found a workaround: it is needed to fully specify
the returned type, not just "iterator" but expanded
So for example the
template<class Ch, class Tr>
typename basic_ptree<Ch, Tr>::iterator
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> >
typename basic_ptree<Ch, Tr>::iterator
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
26. For better visual appearance you may insert few <hr>
into the documentation.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk