Re: [boost] [review] [autoindex] Formal review of AutoIndex

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