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

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2006-04-19 22:39:18

Hi,

I am really busy lately and won't have time for complete review. But since I 
am intimately familiar with this domain, I've decided to take peek on my way 
home.
  First and foremost I would like to remind everybody that we already have 
one library intended to cover this problem domain (completely unacceptable 
IMO - but that's another story). This library not only do not address issues 
of existent solution it's not even comparatively close feature wise. It does 
present some additional media formats. But this could as well be done as a 
add-on to existing solution.
  So my vote is NO, thank you very much.

Here some notes in no particular order from my skimming through docs and 
implementation:

1. Name is really bad. Originally I thought this submission has something to 
do with property_map and was surprised to see what I see.

2. Why would you need fast lookup? In 99.9% of cases each variable is 
accessed single time in a program lifetime. No need to optimize so that you 
program startup time go from 5 mls to 5 mks. You won't notice this anyway 
(And in reality it's not even that big difference. Most of your startup time 
you will spend initiating either network connection or some GUI component or 
similar)

3. Even if you insist on alternative lookup. Why not just use single typedef 
with multi_index instead of all the implementation.

4. Extra functionality (to the one in multi_index) should go in free 
functions not in member functions - it would enhance encapsulation and 
reusability.

5. CLA parser is a joke. It's unacceptable any way you present it.

6. I personally believe that inline implementation in this case is actually 
is rather a disadvantage. I would be perfectly happy to stick to ASCII for 
configuration purposes. Even if you insist on wide char support, I would 
implement it as a thin template wrapper around offline implementation that 
operates with void*. Plus some runtime polymorphism could be used.

7.  ptree_utils reinvent the wheel. Why not use Boost string algorithms?

8. I keep repeating: Traits could not be template parameters. You should had 
have three template parameter KeyType, ValueType and ComparePolicy. The same 
way as std::map is doing

9. Access interface are lacking. If I got string identify subsystem and 
string that identify parameter separately why do I need to concatenate them 
to get to the value

10. General note: the whole design/implementation is unnecessary 
complicated. You general part is about 50k. In Boost.Test utils section I 
have a component with very similar rationale but better design (IMO 
obviously) implemented in 16k.

11. Generating part if the design/implementation is completely unnecessary. 
It may? be useful in 1% of the usage cases and in those cases I would stick 
to some alternatives.

IMO library presenters at best needed to do some comparative analysis with 
existing solution and present it as a part of the review.

Regards,

Gennadiy