Matias Capeletto wrote: ----------------------------------------------------------------------------------------- First of all i want to state that i really like the functionality and interface that ptree offer. As said before the easy of use of this library and the clean code that it generates are impressive. If we have to make a change that compromise the interface i think we are going to kill it. Although, i have a proposal that i think will: a) preserve the actual interface. b) add a lot of functionality with the same easy of use c) give better performance I think that we are missing the big picture discussing about . or / in the get-put functions. The fact is that the library implements the path search directly, and this is a very nice spot to insert one level more of abstraction. A path can be view as a single linked list of keys, the "xxx.yyy.zzz" is only one way to representing it. (a very convenient way i have to said) Think what we could gain if we define a path concept here (i have read all the code of the library and it is a straight forward change to it). The get/put function will receive as a parameter a path, insted of a '.' concatenated key. First we support conversion from '.' concatenate keys to path... Now we can use the ptree in the same way as it is now. Next we support operator / for making the path. The thing is that with a path concept in the middle we can now use the following alternative idioms // Standard conversion // Very clean and the path can be formatted as a hole or can be read from a file. { ptree data; data.put( "debug.info.count", 3 ); } // Other separation parameter { ptree data; ptree::path p(' '); data.put( p / "debug info count", 3 ); // space used as a separator, note that this is cleanner than // pass ' ' as a parameter, and half of the get-put functions in the // ptree implementation dissapear } // Can have a path with '.', '/', ' ' or any other character in the keys { ptree data; ptree::path p; data.put( p / "debug" / "info" / "count", 3 ); data.put( p / "d.e.b.u.g" / "i n f o" / "s.t.u.f.f", 10 ); } // Can use variables without the needs of special concatenation care // Observe we can mix concatenation with operator/ { struct Human {string name; int age; }; typedef vector Humans; Humans humans; ... ptree data; ptree::path p( '/' ); for( Humans::iterator h = humans.begin(), end = humans.end(); h != end; h++ ) { data.put( p / h->name / "info/age" , h->age ); } } // And finally other things can easily be supported // As an example, suppose we need to group e-mails by providers { struct email_info { string direction; string owner }; typedef list eList; eList email; ... ptree data; ptree::reverse_path p('@'); for( eList::iterator e = email.begin(), end = email.end(); e != end; e++ ) { data.put( p / i->direction , i->owner ); } // And now we can ask to the tree things like data.count( p / "gmail.com" ); // Or save this info grouped in an xml! } // Now we can consistenly support other key types { intpTree ipData; intptree::path p; ipData.put( p / 140 / 123 / 25 / 10 , "home" ) } I think that this flexibility is great and i cant see any major drawback of adding it to ptree. Best of all, programs that used the old version will compile without any change. The lookup performance is better because you don't have to make many copies of the '.' concatenated key tails. And if the operator/ sintaxis is used the performance is even better. Some implementations details could change, in my head the put/get function pop the head of the path list while they are using it, and as a post condition always return an empty path. I know it is not very usual, and other more standard way of doing this could be worked it out. Ok, that is it... i hope others like this flexibility too. * What is your evaluation of the design? I really like it, specially the easy of use. I would add the path abstraction to boost ptree flexibility :) 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. * What is your evaluation of the potential usefulness of the library? Very, very high I think this library will be widely used. * Did you try to use the library? With what compiler? Did you have any problems? VS2005, works very nice... * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? Read the documentation, study all the .hpp and think about it quite a lot. * Do you think the library should be accepted as a Boost library? Yes, i will encourage other to use it too. - was the library's performance good enough? If not, can you suggest improvements? incorporate path concept - was the library's design flexible enough? If not, how would you suggest it should be redesigned to broaden its scope of use? again, the path concept regards Matias Capeletto Argentina PD: Congratulations to the library creator! it is very light-weight and powerfull... PD: this is my first libray revision... sorry if there is another aproach to present it. ///////////////////////////////////////////////////////////////////////////////////// Marcin Kalicinski wrote: ----------------------------------------------------------------------------------------- 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, ///////////////////////////////////////////////////////////////////////////////////// Matias Capeletto wrote: --------------------------------------------------------------------------------------- > 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. That is it! I really like the new set/del, seems fairly easy to learn and solve the bool argument problem, adds a lot of functionality too. I have to propose only one more feature, before the picture is complete. If you look in this thread, someone ask for something like "debug.info.testfail[2]" for access repeated tags If we get the path into ptree we can overload other operator to support this type of path (we can not overload operator[]... because of precedence issues :( someone nows a way we can still use it?, the sintaxis will be very nice. As i see it, the only the choosen operator must have the same priority, see the examples, so we have only have * and % ). Something like this may work... ptree data; ptree::path p; data.put( p / "debug" / "info" / "testfail" , "somefile.hpp" ); data.put( p / "debug" / "info" / "testfail" , "otherfile.hpp" ); data.put( p / "debug" / "info" / "testfail" , "wer.hpp" ); data.set( p / "debug" / "info" / "testfail" %2 , "file.hpp" ); // we use set! And we can use it in the middle too, if we have... ------------------------------------- cant find path other error segmentation fault ------------------------------------- ptree data; ptree::path p; read_xml("logFile.txt",data); string swhat = data.get( p / "log" %1 / "what" ): ------------------------------------- What do you think? > 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. I like more the path to be ortogonal to the tree, i think we may need to use differents paths in the same ptree, like a simple path in some point and a reverse_path in other place, or even maybe a relative_ path. Something like: ptree data; for( Humans::iterator h = humans.begin(), end = humans.end(); h != end; h++ ) { ptree::relative_path p(data, h->name + ".info" ); data.put( p / "age" , h->age ); data.put( p / "nick" , h->nick ); data.put( p / "dir" , h->dir ); } But maybe is too flexible... and you can impose to only use one kind of path.