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

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 
formats.

> _____________________________________________________
> 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 
cross-linked.

> 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 
version.

> ---------------
> 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
> http://uk.builder.com/programming/c/0,39029981,20271583,00.htm

>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
> #endif

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 
interface.

>     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 
archive.

> --------
> 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.
Marcin