Boost logo

Boost :

Subject: Re: [boost] [review] [autoindex] Formal review of AutoIndex
From: Barend Gehrels (barend_at_[hidden])
Date: 2011-05-15 13:24:07


Hi Daniel, John, list,

This is my review of AutoIndex.

First of all, yes, I think AutoIndex should be included in the Boost
Toolset so my vote is a YES vote. It is a useful tool, and it is already
used in practice by three Boost libraries, so yes, it should be accepted.

> - What is your evaluation of the design?

I didn't evaluate the design.

> - What is your evaluation of the implementation?

Several questions:
- Why Boost Program Options is not used? It is mimicked here
- If I see this: "if(internal_indexes == false)" I normally ask people
why not write "if(! internal_indexes)" so I ask that here as well
- Why is it indented at 3 spaces instead of the normal 4 as done e.g. in
Boost.Math?
- Why Boost String Algorithms are not used and a function
"make_upper_key" is created
- A copy of TinyXML is used here. Boost.PropertyTree uses RapidXML. It
is really time to have a standard XML parser within Boost. I see from
the copyright notes that this dates from 2002 so this is probably
continuing existing Boost practice, so you might skip this comment
- The method process_node is quite long, might be splitted (the whole
block below comment "Search content for items" is an obvious candidate)
- The method check_index_type_and_placement contains much code
duplication especially in the exception messages

Overall I was not impressed by the implementation, but because it is a
tool and not a library, I think lower code quality is acceptable.

> - What is your evaluation of the documentation?

It is good but not perfect. I've several notes about this, in addition
to the notes that are made earlier by Edward.

About the name: the tool is called AutoIndex. The executable is called
auto_index. We have to use it in the Jamfile by auto-index. The doc
confuses this a bit by stating "full-path-of-executable-auto-index.exe"
and also mentioning autoindex.exe (tutorial/configure)

About quickbook-define: the semi-colon in the doc (tutorial/configure)
should probably not be there. Besides that, it does not work for me (I
get this error: "error: Invalid property '<quickbook-define>': No value
specified for feature 'quickbook-define'.") So probably there is a term
(auto-index?) missing there. Anyway, omitting it let the whole thing
still work.

In the Script File (.idx) Reference, the reflex example is not explained
and it is probably a duplicate of the example "\<myclass\w*\>"

On the same page, the sentence "If this third section selection field
... " is unclear - it is the second regex, the third field, "selection
field" is mentioned here for the first time.

Same page, "functions, classs, macros " typo in "classs" (there is a
myclasss where this is intentional)

The !debug regular-expression should IMO write the "regular-expression"
in italics or something else to indicate it should be replaced

I don't understand this: "If you are writing a Quickbook document with
Doxygen reference documentation, the position of a [xinclude
autodoc.xml] line in the Quickbook file determines the location of the
Doxygen references section" . It probably refers to an autodoc.xml
created by a Boost XSLT tool (I'm not sure). That should be explained. I
didn't dive into this and didn't ask questions on the list about this,
it might be that it is generated by something else or a Doxygen option
or that I just don't understand.

> - What is your evaluation of the potential usefulness of AutoIndex?

Very useful for people writing BoostBook or QuickBook, so the majority
of the Boost library authors and some others.

> - Did you try to use it? With what compiler? Did you have any problems?

Yes. I first tried it by calling the Jamfile. Then I noticed some
compiler errors. Because I now know that bjam gets updated regularly, I
called the booststrap batchfile. Then I tried again and everything
builds fine.

In using and finetuning the index, I noticed that it indexes too much.
That can be helped by adding them to excluded terms. Further, I was not
able to exclude a term from a section. Discussions about this are still
going on on this list.

In general, I do like the produced index, at least for Spirit, Math,
TypeTraits. It provides much added value for library users. Also the
samples (as discussed on this list) being indexed is useful (I only want
to turn off some terms of them).

For Boost.Geometry, there is some added value. To really judge it, I
would have to finetune it more. Currently it contains too much entries,
as discussed. But (as not yet discussed) there are also missing entries.
The term "reverse" contains only entries from the examples. The
"reverse" function itself is not there. If I remove the
reverse-examples, the whole term "reverse" is missing from the index.
Don't know yet why.

> - How much effort did you put into your evaluation? A glance? A
> quick reading? In-depth study?

I compiled it, configured it, used it and debugged it so yes, it was a
kind of in-depth study. I think I spent more than 8 hours on it. Well,
that was not only for this review alone, but to evaluate if it is useful
for the Boost.Geometry documentation.

> - Are you knowledgeable about the problem domain?

Yes.

> I think it should be made clear that if accepted, there won't be any
> requirement to use AutoIndex it.

I think this freedom is essential. Though I tried it and voted Yes, I
still don't know if we will use it in practice. I've to finetune it more
and some things has to be solved, then we can judge the results.

Finally, I want to thank John Maddock for developing this useful tool
and for help on the list, and Daniel James for managing the review.

Regards, Barend Gehrels


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk