From: Marcin Kalicinski (kalita_at_[hidden])
Date: 2006-04-20 07:58:35
Hi Pavel, thanks for a review.
> I took look on the V5 and collected few notes.
First of all, from some of your comments I see you must have looked at the
older revision. Rev.5 does not have any pointers in the interface, and no
longer has get_d, get_b variants.
> Most important, the documentation still needs
> quite a lot of work.
You can always say that about any documentation. Although it should be
polished and possibly expanded in places (like traits customization), I
think it is already quite large (over 150kb of text). It also contains a
reference which covers 100% of interface functions in quite a detail. This
is something that not every boost library has.
> 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.
Agreed. This is not a generic tree container. This library is about
idiomatic access to configuration data, easy reading/writing of many file
> 1. docs: it is rather unusual for me read code
> without syntax highlighting.
The problem is, it is written and maintained in HTML. I could add code
coloring, but that would render it close to unmaintainable. Any change would
be a nightmare. I should have used QuickBook, but unfortunately I had
problems setting up the toolchain. So I stuck to HTML. Btw. some other well
estabilished libraries in boost also do not have code coloring, and I didn't
notice anybody complaining.
> 2. The first mention about use of exceptions in docs
> (in debug_settings::load) should by hyperlink to details.
I agree. I could add some more hyperlinks all over the docs. But whole
synopsis section, and most importantly the reference, are thoroughly
> 3. The first mention of get_d() should explicitly say
> that "d" means default. If possible this word should
> be bolded.
get_d is not longer part of the library, it was removed in rev5. You must
have looked at the older version.
> 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.
I agree, but is it a problem? I don't think it will be an issue if you read
long instead of int and have it implicitly converted. If you want int, you
can always get it by using get<int>(...)
> 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.
Yes, that's right. This is not explicitly said anywhere, but iterators only
get invalidated when element they point to is removed (like std::list).
Insertions/erases of elements do not affect other iterators.
> 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).
I know, you said that before, I have it hovering on my list of things to be
done. It has never got big enough priority though.
> 7. The temptation to use std::lower_bound and similarly
> dangeroud std algorithm may be eliminated by defining
> unimplemented specialisations for ptree
You can always sort the tree and then use the algorithm safely, so banning
it would not be wise.
> In template<class Ch, ....
> the "Ch" should be spelled fully. Ch gives no clue.
> Dtto the "Tr".
No more templates on <Ch> in the library. You must have looked on the old
> 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/
Exactly that was done in revision 5. basic_ptree is now parametrized on key
type (besides other things).
> stable_unique() operation may be added.
> This would eliminate all duplicates while
> keeping order of what is kept.
This can be provided as an generic external algorithm, no need to clutter
the class interface. I don't think it belongs to ptree library, it's rather
an extension to <algorithm> header.
> A generic implementation of stable_unique
>From what I seen the implementation requires random-access iterators, so it
would not work with ptree.
> "It has just one data (typically string) associated..."
The docs are littered with hardcoded references to string as a key/data
type, because customization facilities were added late. Anyway, I think that
using std::strings will be the most common case, and adding parentheses
everywhere I talk about data/key types might do more harm than good.
> 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.
Yeah, my english could use some polishing. The text is probably overly
complicated and littered with meaningless words in places. Again, I would
appreciate if a native speaker skimmed over it and pointed me towards
several most glaring mistakes (grammar/style etc.).
> 15. The headers may have
> #if (defined _MSC_VER) && (_MSC_VER >= 1200)
> # pragma once
I know, you suggested that before :-) Again, this is somewhere on my list of
things to be done, but never got high enough priority. Btw. is compilation
time really shortened that much? I think that it only removes the
preprocessing step, which is done in a snap anyway. I haven't done any
measurements so I might be wrong.
> 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".
The top level folder _only_ contains files which are includable be the user,
all the rest in is details. I want to stick to that.
> 18. ptree_utils: the function widen/narrow are
> horribly inefficient. The "result" string
> should be reserve()d before characters are added.
I know there are performance issues with some of the parsers. I think these
can be resolved safely later, because they do not have any impact on the
> The narrow<wchar_t> is flawed.
Why do you think it is flawed?
> The function trim() is already available
> in Boost.String Algorithms.
I though I'd rather avoid dependencies if they do not provide critical
functionality, like e.g. use of Spirit to parse XML.
> 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.
As far as I know all source files end in newline. I doublechecked
ptree_interface.hpp, and it does end in newline. I think gcc
(with -Wall -pedantic) issues warining if a file does not end in newline,
and library compiles without any warnings on gcc.
> 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.
> The header is missing
> #include <cctype>
>From which file it 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
> over network.
I was actually trying to get a step further: use property tree as a target
(not source) for serialization. I.e. creating Archive which writes to a
ptree. This opens door to many interesting possibilities, for example you
can save your archives as JSON out of the box. I may post some source code
in the future when I get it working. The only drawback to using ptree as a
generic archive is performance, it is going to be much slower than a regular
> 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).
It is safe. The pointer returned points to child which is owned by local,
which is stack based and will be cleaned up with all the children in case of
exception. Btw. this is code from old version of the library.
> 25. I tried to compile the test with BCB (version 5.8),
> from BDS 2006.
I know the library will have problems compiling on non-compliant tools. It
woukd be nice to have it working on old stuff, but it probably requires so
much work that it is rather low on my priority list.
> 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).
That's great. If somebody else with better knowledge of BCB intricacies
could do it, it would be nice. On the other hand I'm afraid of introducing
too many maintenance problems (aka ifdefs), or compromising the
design/performance of the library to make old stuff happy.
Thank you for another great review.
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk