Boost logo

Boost :

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.


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