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

From: Marcin Kalicinski (kalita_at_[hidden])
Date: 2006-04-21 19:59:22

Hi Matias,

Thank you for a great review.

You may have noticed I have already replied to similar proposal of Jeff 
Flinn, somewhere in review thread. Now, as you presented the implementation 
and interface in so much detail, I must definitely reconsider. Previously I 
only thought of a path class as means of encapsulation for default 
separator. And your proposition goes much further. The idea with overloading 
operator / looks very nice to me. It will remove the need for default 
separator altogether (read: halve the number of overloads). It will also 
make the lookup faster.

I'm thinking how to merge it with another important suggestion, which is to 
allow paths of arbitrary type, not only strings (for example arrays of ints, 
where each int is an index on its level, in some cases this might be 
useful). Having the path class, whole path parsing should be done by path 
objects. Probably the best approach would be to templatize basic_ptree on 
path_type and require certain functionality from it. This is already done, 
except that instead of path we have a string functionality that is needed.

> If we have to make a change that compromise the interface i think we
> are going to kill it.

I think you are right.

> Only one thing, i dont like to pass a bool as a function parameter
> because i always forget what was intend for. You use this aproach to
> specify if the path must be overwritted. I thinked about it and not
> reach to any other good sintaxis, but it is there.

There is a different approach, vastly superior to the extra bool parameter. 
Just add "set" functions in addition to "get" and "put". "set" will always 
replace existing property if it is there, while "put" will always add new 
one. The only reason why I didn't implement it is because the get/put 
interface was already bloated, and adding "set" looked like a step in wrong 
direction. Now, that I think we can get rid of those ugly separator 
overloads (sigh), this may be an option again.

Additionally the library should have a get/put-style member function to 
delete an existing property. It obviously has erase, but it does not work 
with paths. We would end up with get/set/put/del(?), and the picture would 
be, hopefully, complete.

Again thank you for a great review and best regards,
Marcin