"Thorsten Ottosen" wrote:
I have the great pleasure to announce that the formal boost-review of Marcin Kalicinski Property Tree Library
I vote weak yes to accept the library. Personally, I would prefere to wait for rev6 until some issues get resolved. Given that the review had started I think it is better to accept the library as it is very likely to get maintained and improved. Not accepting it would mean 6-12 months delay and possibly abandonment. I did a review of rev5 few days ago - the notes are pasted bellow if anyone is interested. /Pavel ------------------------------------------------------------------- Hello Marcin, I took look on the V5 and collected few notes. Most important, the documentation still needs quite a lot of work. 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. Ptree 's main value is its simplicity for its intended task and this should not be compromised. /Pavel _____________________________________________________ 1. docs: it is rather unusual for me read code without syntax highlighting. _____________________________________________________ 2. The first mention about use of exceptions in docs (in debug_settings::load) should by hyperlink to details. I would welcome every code snippet to have such link, very visible to people who co copy + paste. _____________________________________________________ 3. The first mention of get_d() should explicitly say that "d" means default. If possible this word should be bolded. _____________________________________________________ 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. There may be some interesting side-effects with this. At the moment I do not see much use for the distinction or some idiom based on it but perhaps it may be worth of some attention. _____________________________________________________ 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. _____________________________________________________ 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). For example a huge structure that is processed sequentially may not need it (and would be better with more memory). _____________________________________________________ 7. The temptation to use std::lower_bound and similarly dangeroud std algorithm may be eliminated by defining unimplemented specialisations for ptree (in std:: namespace). This is allowed by standard. _____________________________________________________ 8. docs, "Synopsis": "Instances of this class are property trees." - the sentence may be better reworded, it sounds somehow redundant to me. ------------ The mention of ptree, wptree, iptree and wiptree should be hyperlinked - it is absolutely unclear what they are about. ------------ Perhaps tables could be used as a tool to combat the cryptic abbreviations. These tables would have list these abbreviations and rationale for the name. The docs hyperlinks may point to such tables for quick overview.
From the table one would be able to go deeper into references.
--------------- The lines class ptree_error; class ptree_bad_path; class ptree_bad_data; should have comment that these classes are actually exceptions. It is not that clear. ------------ The name "ptree_error" would be better "ptree_error_base" to give immediate clue ----------------- empty_ptree() should be create_empty_tree(). or make_empty_tree() (I prefere the first, the second is somehow semistandard). -------------- In template<class Ch, .... the "Ch" should be spelled fully. Ch gives no clue. Dtto the "Tr". --------------- 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/ wxString/boost::fixed_string/etc and the library should not lay obstacles to them. Another possible template parameter is allocator. --------------- stable_unique() operation may be added. This would eliminate all duplicates while keeping order of what is kept. A generic implementation of stable_unique can be seen on http://uk.builder.com/programming/c/0,39029981,20271583,00.htm -------------------- The part of docs named // Ptree-specific operations should have more comments. It is almost unreadable blob as it is now (no syntax highlighting, no hyperlinks to actual code). ------------------- get_own() etc: I suggest to add table of abbreviations on the top of documentation, before anything else. I feel rather stupid looking on "own" and guessing what could it be (I try to feel as first time reader). ---------------- OTOH the locale is not needed to be described in such detail in the documentation. Just someting as --locale-- should be enough to get clue. --------------- Instead of "class CharType" a "typename CharType" feels better. It is also somehow confusing what suddenly new character type appears here and what it could mean in the design. There should be commen, possibly link to example. _____________________________________________________ 9. docs, "How to populate property tree" The word "parsers" should not be used. It brings feeling of Spirit or yacc. A standard and not overloaded word is "reader" or "reader/writer". -------------- "It has just one data string associated..." ==>> "It has just one data (typically string) associated..." ----------- The sentence of what parts of XML (as multiple props) are not supported should make it into section of its own and this section should appear in top table of contents. ------------- Existence of file_parser_error is not shown in the synopsis. There should be picture, class diagram of all existing exceptions, possibly as clickable map. ------------- The <xmlattr> discussion is now strangely splitted among two sections (I am not able to distinguish if they are or are not of the same level). ----------- The sections may be numbered with x.y stype. _____________________________________________________ 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. _____________________________________________________ 11. docs, "JSON parser": JSON could by hyperlinked to external website. _____________________________________________________ 12. docs, "Command line parser" - existence of Boost.Options library may be reminded in the begining of this section. _____________________________________________________ 13. docs, "How to access data in property tree ": "Property tree resembles (almost is) a standard container..." is very ambiguous as there are many standard containers. _____________________________________________________ 14. docs: few examples may use wide strings als L"...". _____________________________________________________ 15. The headers may have #if (defined _MSC_VER) && (_MSC_VER >= 1200) # pragma once #endif on the top to reduce a little bit compilation time for VC and Intel. _____________________________________________________ 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". _____________________________________________________ 17. Idea of a feature: right now the ptree is intended as temporary structure - read from file and then transformed into user data. People may like to use ptree as primary structure, without need to define their own helper classes. For this it would be useful to be able to attach some data to nodes. I suggest to add yet another template parameter or type traits: associated datatype, defaulting to void. If they are not void then this datatype will exist next to each string. (Boost.Any may be useful example.) _____________________________________________________ 18. ptree_utils: the function widen/narrow are horribly inefficient. The "result" string should be reserve()d before characters are added. Since the only char types known to a man are singned char/unsigned char/char and wchar_t it would be better to provide specialisations for these. If someone wants something strange (string of doubles) he would need to write a meaningful specialisation for it. The narrow<wchar_t> is flawed. The function trim() is already available in Boost.String Algorithms. _____________________________________________________ 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. _____________________________________________________ 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. _____________________________________________________ 21. json_parser.hpp, create_escapes(): a switch should be used instead of the if-else chain. ----------- The header is missing #include <cctype> _____________________________________________________ 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. _____________________________________________________ 23. cmdline_parser.hpp: there may be support for parameters passed via WinMain() - i.e. as single string. The code that splits the string into argv tokens already exists in program_options library. -------- 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). I have feeling an auto_ptr may be useful here. _____________________________________________________ 24. The docs should show exception safety level for every member function. (e.g. as smart_ptrs have). _____________________________________________________ 25. I tried to compile the test with BCB (version 5.8), from BDS 2006. The problem is that Borland doesn't like the out-of-class member bodies that return any kind of iterator (begin/end/rbegin/rend/front/back/find/erase/push_front/push_back). I found a workaround: it is needed to fully specify the returned type, not just "iterator" but expanded definition: So for example the template<class Ch, class Tr> typename basic_ptree<Ch, Tr>::iterator basic_ptree<Ch, Tr>::begin() { return m_impl->m_container.begin(); } would need to be changed to ugly: template<class Ch, class Tr> #if BOOST_WORKAROUND(__BORLANDC__, BOOST_TESTED_AT(0x564)) std::list<std::pair<std::basic_string<Ch>, basic_ptree<Ch, Tr> >
::iterator #else typename basic_ptree<Ch, Tr>::iterator #endif basic_ptree<Ch, Tr>::begin() { return m_impl->m_container.begin(); }
Perhaps it is worth of have BCB support. The compiler is quite shitty but their IDE is so far the best C++ RAD environment on the planet. Some people use it for this reason. 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). _____________________________________________________ 26. For better visual appearance you may insert few <hr> into the documentation. _____________________________________________________ EOF