Re: [Boost-docs] [quickbook] Direct code import...

Subject: Re: [Boost-docs] [quickbook] Direct code import...
From: Daniel James (dnljms_at_[hidden])
Date: 2011-10-29 12:34:07

On 29 October 2011 03:48, Rene Rivera <grafikrobot_at_[hidden]> wrote:
>
> [import ../../../boost/predef/version_number.h]
> [import ../../../boost/predef/architecture/.*\.h]

I still don't like this much, but since it looks like this will be
forced on me, here's a review:

It would have been better if you'd branched from quickbook-dev, as
there are some changes to import/include there that are very relevant.
There's the conflicting include source file thing that I implemented
there but it should be pretty easy to adapt to your purposes. If you
try building the example I pointed you to, or even just looked at
'gold' test output, it should be obvious what it does.

I'd rather use 'include' than 'import', the idea being that 'include'
is for including content, 'import' is for importing templates and
macros. This should mean that 'include' wouldn't import the other
snippets in the file. With your change it isn't clear what an 'import'
does without looking at the imported file.

'quickbook-dev' has a 'call_code_snippet' function which is a lot
easier to use than 'do_template_action'.

I'd rather use explicit syntax to distinguish between paths and
patterns. This change breaks the windows path support, which I think
means that it can no longer be used to build some older versions of
the maths documentation (I want to phase out support for windows
paths, but try to keep quickbook backwards compatible, that's why I
just added a warning for now). I also plan on implementing escapes for
import, include, xinclude etc. eventually (1.7), which might interact
badly with this, requiring double escapes, while an explicit syntax
would indicate that escapes need to be treated differently.

I'd also use a version switch as well. I think things like this should
be approached with some caution.

I'd rather use globs than regular expressions, it isn't that hard to
implement a glob matcher (I think I might have one I wrote somewhere)
and I find it nicer for matching paths. Would also not require adding
an extra dependency.

I haven't tested it, but I don't think your code will work on windows.
For example, the line:

    std::string f = i->path().filename().native();

should probably be something like:

    std::string f = detail::path_to_generic(i->path().filename());

I think it'd be better to return 'std::set<include_search_return>'
than turn 'include_search_return' into a stack. It took me a little
while to work out what you were doing. Actually, I'd probably go for a
vector and just sort it. I admit that 'include_search_return' is a
really bad name, I wouldn't be against renaming it.

If there are matches at different points in the search path, they're
ordered by absolute path which may vary on different computers. I
think my preference would be to keep them in the order they appear in
the search path, with 'filename_relative' as a secondary sort field in
case of collisions. Or perhaps 'filename_relative' could be the
primary sort field, and the include path order the secondary sort
field.

If you expand the '!' snippets in a separate loop after creating the
other snippets, I think it would allow to you to expand code snippets
from later in the file which should be useful.

I wouldn't call this literate programming, as I think it annoys
literate programming enthusiasts. See:

http://www.perl.com/pub/tchrist/litprog.html