John Torjo wrote:
The FORMAL Review of "Output Formatter" library begins today, Sept 12, 2004.
Before going to review itself, I'll list the cases where I want to use this library. The primary case is debugging. I want to either output small vectors to some log/dump file (in which case the output will be small one-line), or output some huge structures (vector<Function>, where Function has a lot of data). In the latter case, the output should be multiline, with nice indentation, or I won't understand anything. The second case is STL I/O itself, for example csv files that Reece has mentioned. A particularly interesting question is how the proposed library overlaps with serialization. When outputting vector<Function> I'd prefer the content of 'Function' to be outputted too, preferably by describing the members with the 'serialize' method. And the question is if I can use outfmt library, or have to use the serialization library? The serialization library is pretty large, so, I'd like outfmt to be able to output UDT which have 'serialize' defined. IMO, vector<some_UDT> is a very common case, maybe even more common than vector<pair<int, int> >.
1. What is your evaluation of the design?
Unfortunately, I cannot comment much on this due to bad documentation. See below. There are some things I don't like. First, as many mentioned, the naming is not optimal. For example: boost::io::formatob(v).format(" | "); The 'ob' suffix and two "format" words are confusing. I'd suggest something like: boost::io::stl(v).separator(" | "); or boost::io::separate_with(v, " | "); and for braces something like using namespace boost::io; braces("[", "]", separator(" | ", v)) or maybe even ("[" + io(v)/"," + "]") for nested formats, something like ("[" + io::format( "(" + io::format()/":" + ")" )/"," + "]" ) These are just ideas, though. It's desirable that the library support some multiline output style out of the box, so that I could write: os << io::multiline(v) << ... Again, I'd suggest YAML as such style, but any other readable indented style will be OK. It's also desirable to use the 'serialize' method of classes to output them, instead of requiring operator<< to be always present.
2. What is your evaluation of the implementation?
There should be the <boost/outfmt.hpp> header, including everything else of the library. Some of the lines are longer than 80 characters (e.g. template header of formatob_t has 111 characters). Methods defined in the body of the class are implicitly inline, there's no need to put the "inline" in front of them, as it's just one more word to read. I don't think defining methods in the class is a good idea -- this makes the class interface less obvious. Another issue is that all of methods are inline, and I don't think it's necessary. The iostream operations will take much more time than function call anyway, and inlining everything can lead to code bloat. For example list_object::read is definitely very large method. Why is the formatob_t necessary? It seems to work by delegating everything to the underlying formatter. Can't the 'formatob' just return the appropriate formatter? I might be missing something, but the mechanism for getting the type of formatter from a type to be output seems too complex. First, the type_deducer.hpp file is used, and 'select' computes a 'category'. Then format_deducer.hpp takes the category, and again uses 'select' to obtain the real type of formatter. Why the type_deducer.hpp is needed?
3. What is your evaluation of the documentation?
The documentation leaves much to be desired. I'll walk though some of the aspects (indented text is quote from the docs) First, about the structure. I'd prefer more conventional introduction/detailed docs/reference structure. The current docs, IMO, mix everything, and when reading sequentally, I find both vague phrases and exact synopsis, and very little overview or tutorial material. "providing an extensible framework that sits" How it's extensible? I see only one section about extending and it's just one paragraph long It is often necessary to override the way a type is formatted (written to/read from) to a stream, or to format a subrange of a container or array. The manipulators in this library serve this purpose. For example: int a[] = { 5, 4, 3, 2, 1 }; std::cout << boost::io::formatob( boost::io::range( a, a + 5 )); std::cin >> boost::io::formatob( boost::io::range( a, a + 5 )); I don't think I see a conventional meaning of "manipulator" here. The formatting style is not changed at all. Just a 'range' function is used to create a container from an interator range. boost::io::formatobex< DelimeterType >( const T & ob ); This will format ob according to it's underlying type. How is this different from using the 'formatob' function. What's "underlying type". The "This will format" phrase is very loose, it does not even say anything about stream. I'd suggest: the returned object can be output to a stream, and will produce textual representation of 'ob', or something equally explicit. boost::io::formatob( const T & ob ); This will format ob according to it's underlying type. I'd suggest that this is documented before 'formatobex'. I'd also suggest for formatobex use the "basic" prefix to indicate it's templated on the character type. E.g. stlio and basic_stlio, or something like that. std::cout << boost::io::formatobex< std::string >( v ); // output: [ 1.1, 2.2, 3.3 ] Note that the type construct is automatically deduced and the corresponding format object is constructed. Again, this is very vague. What does it mean for "type construct" to be automatically "deduced". Why would I care about some "format object". You might want to say that under the cover, the type of 'v' determines the type of format object that's created, and that the object is responsible for the actual formatting. This will format ob based on the format object it is passed (FormatObject). Here, the format type is taken from FormatObject::format_type. This allows the nested constructs to be formatted. This is not clear at all. What's 'format type' and how it's "taken". How it allows the nested constructs to be formatted. I think you need to either elaborate on this, or move this passage to a detailed docs section. If you need to specify a range or sub-range, boost::io::formatob will not recongnise it unless it is a container. boost::io::range( ForwardIterator first, ForwardIterator last ); This is more of a language problem. Range, by definition, is a pair (but not std::pair) of iterators. It's never a container. This creates the range [first, last) that can be used by boost::io::format. The range [first, last) exists as long as you have the 'first' and 'last' objects. I think it's better to say: "Returns an object which will output the range [first, last) to the stream....". The remainder of the "range" function overload leaves me wondering if this is reference or overview or what. For a reference, the code examples are not necessary. For tutorial, you don't need to list every overload, just mention that there are others. A FormatTraits class is a class that provides the default values for the open, close and separator delimeters used when rendering the list to a stream. These can be overriden by the user as described in the following section. The class has the general form: This sentence doesn't say why FormatTraits is usefull for a user. I read it like: the class provides default values, and user can override those value when outputting a specific object. It does not seem that the user can specialize the class for his objects, so why the user should know about this class at all? Heh, both provided traits are in the detail namespace? Really, user should not care. boost::io::openclose_formatter is a class that allows the user to change and access the format used for open and close delimeters Here we're definitely in the reference docs already, while I did not get an overall picture yet. Then, what's "change and access the format". If the format can be changed, it is stored somewhere. Where? The code block before this comment defines two classes openclose_formatter_t and openclose_formatter. Is this a typo, or you really have two classes? boost::io::openclose_formatter_t is used by format objects so that a reference to the format object is returned and not a reference to boost::io::openclose_formatter! This makes it possible to call the format function inline. For example: It's completely unclear. Then you go on describing the openclose_formatter class, while I still don't know how I can use that class. Formatters are useful when you want to use a specific format in different places, for example: This should be the first sentence in this section. The FormatObject class has to provide a write function having this syntax: FormatObject::write( os, fo.ob ); Maybe, you mean "The FormatObject class has to be written such that expression FormatObject::write(os, fo.ob) is well formed". My first impression was that you give a signature for the write method. The write function has the form: template< typename T, class OutputStream > inline OutputStream & write( OutputStream & os, const T & value ) const ( // ... return( os ); ); Do you mean that every FormatObject class should have this signature? Then, the phrase about "FormatObject::write(os, fo.ob)" is not necessary.
4. What is your evaluation of the potential usefulness of the library?
Potentially very usefull.
5. Did you try to use the library? With what compiler? Did you have any problems?
No.
6. How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
A couple of hours.
7. Are you knowledgeable about the problem domain?
Kind of. I wrote a similar library a few years ago, though much less simple.
And most important, 8. 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.
I'd a hard question. The basic idea of the library -- formatter objects that you can nest, seems sound to me. However, the interface built on top of that is a bit suboptimal, and the state of documentation is pretty bad. Because of that, I believe that it would be better if the library is rejected this time, and reviewed again later. - Volodya