Re: [boost] Review request: auto-index tool

Subject: Re: [boost] Review request: auto-index tool
From: Bryce Lelbach (admin_at_[hidden])
Date: 2011-01-21 09:01:39

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John, please find attached a patch plus a short summary of notes. More extensive
comments:



Jamfile
- -------

While auto-index is still not a part of boost, I think you should check for the
environmental variable BOOST_ROOT so that users can checkout auto-index and use
it outside of their boost-trunk source tree. I see that you had:

 local boost = [ modules.peek : BOOST ] ;

I've never seen the above done before to locate boost. I usually use:

 import os ;
 local BOOST_ROOT = [ os.environ BOOST_ROOT ] ;

I'm sure volodya could tell us which is more portable.

Other problems with the Jamfile; the targets for rgeex, filesystem and system are
bad for GCC and clang (copious linker errors). Instead of:

 $(boost)/libs/regex/build//boost_regex
 $(boost)/libs/filesystem/build//boost_filesystem
 $(boost)/libs/system/build//boost_system

you should have:

 $(boost)//regex
 $(boost)//filesystem
 $(boost)//system

This leads to a successful build for me with gcc and clang. This is how the wave
tool specifies libraries to be linked, so I'm assuming this will work fine on
MSVC, etc.

Source Code
- -----------

The long initializer for the 257-element boost::array<> at line 168 in
auto_index.cpp is painful to look at, and raises warnings with clang. I suggest
reformatting it. A few other places in the code have painfully long lines (see:
attached patch).

Why aren't you using program_options for the command line argument parsing? Is
there a particular rationale here, or is this something you didn't consider?

All in all, the code does what it's supposed to (we've experienced no
bugs/problems with this tool in the Spirit docs yet that I know of). However, I
do have reservations about the design of the tool, though I suspect these
reservations may be entirely stylistic. I have similiar reservations about
other Boost tools, so perhaps I am biased.

I feel like this code, while usable, is not particularly extensible. It feels
like (no offense) a typical C application; copious use of source-file local
global-namespaced functions and globals instead of tying said functions and
globals together into classes which could be reused and hard-coded defaults
preferred to more generic, configurable code. This is intended as a tool, and
not a library, so this is understandable, moreso because the tool does exactly
what it's supposed to do.

On the other hand, I'd rather see a tool that is a library. An excellent
example of this design model is the Clang project. Clang is a C language
compiler, and at first glance it would appear to be "application" software.
However, it is, in fact, "library" software; Clang is, first and foremost, a
set of libraries which are designed to be reusable and extensible. The Clang
driver (aka the "tool") is just an application that makes use of the Clang
libraries.

Finally, as a Spirit developer, I feel the need to make a shameless plug for my
library :x. I wonder if some of the regex code used in auto_index (in
particular, the stuff in file_scanning.cpp) wouldn't be better implemented with
Spirit or Xpressive. I see a lot of regexs hard coded in strings, which implies
a runtime cost to parse those strings... that cost could be possibly be avoided
by rewriting that code as expression templates with Spirit or Xpressive. OTOH,
that's probably more trouble than it's worth.

Finally, another optimization thing - I've noticed for the Spirit docs (193
terms), indexing can take a while. Nothing too excessive, but certainly a
noticable amount of time. As auto-index seems to currently work as intended,
and there appear to be no bugs in it, perhaps investigating some optimizations
to increase the speed of indexing would be prudent?



I hope this feedback is helpful John. This is a great tool, and it has
excellent and extensive documentation. I would hope that it would ship with
Boost 1.47.

Sincerely,

- -- 
Bryce Lelbach aka wash
boost-spirit.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)

iEYEARECAAYFAk05kcMACgkQ9cB/V3/s9ExzNgCghvdSB9TGbREtPqPf8Dldv/xp
8OoAn0ok5u76QHhiSVyO6H4TDvHrlz8l
=gBBq
-----END PGP SIGNATURE-----