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

Subject: Re: [boost] [review] [autoindex] Formal review of AutoIndex
From: John Maddock (boost.regex_at_[hidden])
Date: 2011-05-16 05:08:40

> Several questions:
> - Why Boost Program Options is not used? It is mimicked here

In the beginning I didn't think there were going to be enough command line 
options to require it..... and then they grew somewhat...

> - 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

Taste I guess, I generally prefer the explicit comparison <shug> I guess.

> - Why is it indented at 3 spaces instead of the normal 4 as done e.g. in 
> Boost.Math?

All my stuff uses 3 spaces, always has.  Hangover from the influence 
Borland's OWL (anyone remember that?).

> - Why Boost String Algorithms are not used and a function "make_upper_key" 
> is created

Good question, will look into it.

> -  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

Amen to an official Boost XML parser!

Actually the TinyXML version used has been modified for this particular use 
case...

> - 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.

Nod.  It's currently typical of what happens when you don't know where the 
destination is when you start :-(

There's a lot of room for refactoring/tidying up once it's clearer where the 
tool is headed, and what features are needed.

>>     - 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)

Nod.  I got a bit skitzophenic about the name.  Will fix.

> 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

Will add to the fix list.

> 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.

IMO It doesn't belong there - it was added by a pre-review reviewer.

>>     - 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.

Will investigate.

>>     - 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.

One of the issues with a tool like this is that it was developed with 
certain documentation in mind.  Basically it needs to be tested/used by 
other docs writers to find out what's missing or needs to change for maximum 
versatility.  So your feedback is most welcome.

>> 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.

And thank you very much for the in-depth review, very much appreciated!

Regards, John.