|
|
Boost : |
From: Ivan Vecerina (ivec_at_[hidden])
Date: 2006-04-19 18:44:00
"Thorsten Ottosen" <thorsten.ottosen_at_[hidden]> wrote in message
news:44453A4C.2040302_at_dezide.com...
: * What is your evaluation of the design?
First I apologize for having no awareness of the lib's history.
This is also my first review [ I usually don't have time
for that, does it suffice to say that I have 4 small kids? ]
I will highlight with ** (on the beginning of the line)
each question/point for which I would appreciate an
answer or discussion or action.
I'd say at first glance that it felt a bit XML-centered
[ which is neither good nor bad, but the reflection of
some design choices, and wanting to be very general ]
My vision of a property-tree is more of a "node" base class
with three subclasses:
- Leaf -- which stores actual data ~= string
- Array -- which includes an array ~= vector<node>
- Record/Dictionary/Object -- named fields ~= map<string,node>
This is how most scripting languages support as building
block for any data structure -- and why you find this at
the core of JSON ( http://www.json.org/, probably a must-read
for any reviewer).
I dislike the fact that any 'tree' has a default value,
that 'leaves' are trees, that a node can be an hybrid of
an arrray and a record ( contain both unnamed or other
repeated fields, and structured named fields).
I prefer a design that imposes a minimal level of
validation of the structure of the data (like I prefer
xhtml to the excessively 'loose' original html) --
I think that the result is more maintainable.
An example issue:
** [Based on Tutorial] what happens when I call
pt.get<std::string>("debug.filename");
if there is two 'filename' fields under 'debug' ?
This should generate a failure (throw an exception),
and this should be explained in the documentation.
Also, I would have preferred to have a better built-in
support for arrays. As it is, one can use a "path" to
directly access a child -- except if an array is involved.
In my use of related libraries, arrays are very common,
either as object lists, or as "leaves" (e.g. vector coords).
I find this asymmetry (support for Records > Arrays) disturbing.
Regarding iteration: In similar libraries I have been using,
I liked having support for 'streaming' data or visiting the
whole tree as a whole. Basically being able to flatten
the 'tree' into a sequence of data fields.
Having such an infrastructure at the core of the library
makes it trivial to implement a number of i/o formats - see
the reference to the DataTree design discussed below.
This will also simplify cross-tree operations such
as merge/intersect/override -- which you suggest as extensions.
: * What is your evaluation of the implementation?
Independently of the above design decisions, there are some
things I would do differently for the library's interface:
** I would prefer the member-function interface to be minimal.
ptree reminds me much of std::string, with too many overloads.
Overall, a ptree is nothing more that a collection of named
sub-tree, and its 'own' data field.
I would only include getters and setters for these two fields,
and leave all the rest to non-member functions, because
different users may want to implement different interfaces.
Keep things orthogonal and avoid multiplying overloads.
** 'separator' should not be a data member. This mixed usage
of default/non-default separators only feels like a mess.
Choice of separator is an access/interface method choice,
not a property of the stored data.
** 'find-by-path' functions would be a set of non-member
functions. Implement them with a default-separator
choice (or make an internal but documented version
that takes the separator as a param for those who
want to customise their separator and use their
own inline-forwarder functions).
** By the way, why not include support for arrays in:
your paths - for example: get(pt,"debug.modules[0]");
** the get/get_default/get_optional should be a *single*
set of non-member functions applying to a ptree,
or ideally maybe an optional<ptree&>.
** I have a similar library that provided template-overloads
of node >> T and node << T , which can be
a convenient notation for conversion/extraction.
Specializing these template overloads is how you define
custom converters for your built-in types.
** There should be a way to specialize get/put functions
in a way that translate e.g. vec3f (3 float coordinates)
to/from a ptree ( which I will implement as a 3-value
array). Or from a struct to/from a ptree?
How do I do implement this?
The least is to have a plan defined for supporting this.
The above would make the interface much cleaner IMO.
It offers increased flexibility/customizability to
end-users. I don't think that this has to impair
usability.
If I try to rewrite the "tutorial" based on the interface
I envision, it could look like:
reading:
using boost::property_tree::ptree;
ptree const pt = read_xml( filename );
//BTW: Should a value-returning version of read_xml()
// not be provided? Some care more for convenience
// than performance. And hopefully we will have
// R-value references soon...
m_file = get<std::string>( access(pt,"debug.filename") );
//or: get(pt,"debug.filename") >> m_file;
//or: pt % "debug.filename" >> m_file; // too funky ??
m_level = tryget( access(pt,"debug.level") , 0 ); // for ex.
writing: // simple overall anyway...
put( pt, "debug.filename", m_file ); // or whatever...
: * What is your evaluation of the documentation?
Besides the error-handling for ambiguous paths mentioned early
on, I would like to see:
** a description of the mechanism used to convert ordinal
values into 'data'. I assume this is simply ostringstream,
but I haven't seen this specified anywhere.
** Can you provide examples of the representation of
common types (e.g. bool(true/false), char, int,
double(precision?) )
** If I want to implement a custom 'translator' (value to
text and text to value) for a custom type (e.g. "BigInt"),
what template functions do I need to specialize?
Can I specialize this independently from istream/ostream ?
[ and again, can I define a struct<->ptree conversion ? ]
: * What is your evaluation of the potential usefulness
: of the library?
Very high, obviously.
As it is, this lib is already much better (IMO) than many
other similar libraries I've seen (xml and other).
: * Did you try to use the library? With what compiler?
: Did you have any problems?
No, just reviewed the docs and sources. Too little time, sorry.
: * How much effort did you put into your evaluation?
: A glance? A quick reading? In-depth study?
In-depth look for few hours.
: * Are you knowledgeable about the problem domain?
I have personally written a similar C++ library about 12 years ago,
which I have refined and iterated several times. I had been calling
it "DataTree" (but PrepertyTree is a much better name).
The three companies I previoiusly worked at still use it...
I even once started writing a version of this library in C,
which is available at http://ivan.vecerina.com/code/datatree/ ,
and illustrates some of the core concepts (but obviously not the
interface itself, which can be much nicer with C++ -- beware
of the ugly C idioms in this posted code...).
I would say that the C++ version of DataTree has pros and cons
compared to ptree. [ sorry, I don't see how to post the C++
version as it relies on a number of proprietary libraries... ]
Some of the key design differences include:
- A Tree streaming or traversal system is built-in
A tree can be transferred as a sequence of "tree steps".
A tree can be built from such a sequence, or traversed as such
a sequence (using a visitor pattern). A 'validator' class is able
to verify the consistency of the sequence of tree steps (on-the-fly).
This makes it *very easy* to create a reader/writer for a number
of file formats (you don't need to check the grammar/consistency
in each parser, it is all done in one place).
>> This would be a nice-to-have; it eventually could be implemented
later as an internal add-on to the current design.
- 3 types of nodes are distinguished (leaf,array,record)
Basically, these are 3 subclasses of a common 'node' base class.
The fields of a record are (unique and ) sorted by name -- which
might be better for performance if large records are used.
I prefer having this more restrictive distinction between
leaves, arrays, and records. But being more 'loose'
also helps ptree support closer compatibility with some
formats (Windows registry, XML).
Is this desirable? YMMV.
>> Having either an std::map<string,node> or an std::vector<node>
could afford better performance, or be easier to implement.
Do potiential users of ptree want this additional flexibility?
I guess so.
Should it be possible to check that the structure of a ptree
instance obeys stricter structural rules?
Maybe, but not critical, probably
- all node types use built-in reference counting (you can
therefore make shallow copies of a tree if desired, and
sub-trees can be passed to other objects that store them
without copying).
>> This made sense with polymorphic base class approach I took,
but ptree's current approach is good as it is.
- I implemented a couple of binary and scrambled file formats too.
This *is* convenient when you don't want users to mess with
a configuration file. Checksums are also a requirement when
storing potentially critical data (I do medical stuff).
>> Adding new formats to ptree is easy for end-users to do.
- I don't use iostreams for I/O but always byte-oriented streams.
I consider the internal format to be fixed and byte-oriented.
I also use memory-mapping and platform-specific tricks for most
of the file I/O, because performance mattered in my applications.
The encoding from C++ values to bytes is determined by the top-level
value i/o functions. The default converters use stringstream,
but they can be specialized at well for custom types.
Users may also use a different set of converters
(it would be possible to use binary formatters instead of text).
>> ptree's approach is more conform to the boost approach.
- With the above choices, the core library is not templated.
Only the value<->node conversions are done in template code,
and only this can be specialized.
Implementation details are hidden, compile times are faster.
>> ptree's approach is more conform to the boost approach. ( LOL )
- like 'archives'(IIRC) in boost::serialize, I have a system
allowing to write a single template function that will
either export or import values to a tree.
>> This would be an interesting future add-on for ptree.
- I never bothered to implement path-based access to heirs.
I always wanted to do it. But in practice, I have never needed it.
All in all, my C++ DataTree library design is more 'classic':
more OOD, less templates (except for interface/usability magic),
more performance-oriented (did matter for me).
But less general.
: And finally, every review should answer this question:
:
: * Do you think the library should be accepted as a Boost library?
: Be sure to say this explicitly so that your other comments
: don't obscure your overall opinion.
YES - but with requests for modifications.
ptree is superior to most efforts I have seen.
As expressed in my above comments (**), I feel that the interface
needs to be made leaner, to improve flexibility/customizability.
I haven't gone into the details of how to break-up the orthogonal
aspects of the interface ( 3-kinds of get, child access, path
access). But I would strongly recommend to explore & do this,
as it will matter to make the library more 'evolvable'.
In the same line, I also would like to see a clear documentation
of how the value<->tree conversions are made, and how they can
be specialized.
My gripe regarding the 'too loose' structure is a personal peeve,
I understand well that many would not care.
: In particular, consider if you can answer the following questions:
:
: - was the library suitable for your daily xml-tasks? If not, what was
: missing?
It's good that XML is supported as a text format too.
But honestly I don't care.
: - was the library's performance good enough? If not, can you suggest
: improvements?
Performance should be ok for most users. Just as for iostreams,
it might be a gripe that will come up periodically. But it shouldn't
be a showstopper.
: - was the library's design flexible enough? If not, how would you
: suggest it should be redesigned to broaden its scope of use?
Too flexible in my personal opinion.
This is my very first review, and this is the end of a long work day.
I don't have the strengh to review what I wrote -- please forgive me.
Kudos to Marcin Kalicinski, and to any others who have contributed
to this excellent work. Only because it can be so important,
this library is worth perfecting...
Kind regards,
Ivan
-- http://ivan.vecerina.com/contact/?subject=NG_POST <- email contact form __________________________________________________ Do You Yahoo!? Tired of spam? Yahoo! Mail has the best spam protection around http://mail.yahoo.com
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk