Re: [jamboost] Re: Re: Resource files support (v2)

From: Vladimir Prus (ghost_at_[hidden])
Date: 2004-06-07 02:28:54

Hi Alexey,

> > 1. The transform and replace methods really don't belong to
> > "res-scanner". It would be much more reasonable to add them to the regex
> > module. As for replace, I think it is possible to extend regex.replace to
> > accept list, not string as the first argument.
>
> Few notes:
>
> I changed regex.transform so it accepts third parameter 'indices' now. If
> 'indices' ommited transform will work as before. But I had to comment out
> the string:
>
> NATIVE_RULE regex : transform ;
>
> otherwise bjam didn't see 3rd parameter. I do not fully understand what it
> means. Does bjam support it natively?

Yeah. There's C implementation in jam_src/modules/regex.c which is activated 
by the NATIVE_RULE. I'll take a stab at updating it soon.

> Also I placed my 'replace' to regex module as 'replace-list' rule.
> 'regex.replace' is already used in 'escape-html' rule (print.jam) and its
> prototype cannot be changed without changing print.jam. And I think it will
> be more efficient to use the former replace here as soon as 'escape-html'
> performs several replacements in each string. Or we can write something
> like a generic replace rule that would be able to perform several
> replacements in each string like:
>
> rule replace ( list * : match : indices * : replacements * )
>
> where 'indices' is a list of paranthethised groups of all successful
> matches to replace and 'replacement' is a corresponding list of replacement
> strings. But I guess we may postpone it while we really will need it.

Right. I think the current replace-list is fine.

> BTW: I added two test cases to 'regex.__test__' rule. Both asserts test
> added functionality:
>
> assert.result a.h b.h c.h
>
> : transform <a.h> "b.h" <c.h> : <([^>]*)>|\"([^\"]*)\" : 1 2 ;
>
> assert.result "-" "a-b" : replace-list "&" "a&b" : "&" : "-" ;

Thanks! This is really good to add unit tests with new functionality. However, 
I get this on running tests:

/home/ghost/Work/boost-rc/tools/build/v2/util/regex.jam:158: in __test__ 
from module __test-regex__
error: assertion failure: [ transform "<a.h>" "b.h" "<c.h>" :
"<([^>]*)>|"([^"]*)"" : "1" "2" ]
expected: "a.h" "b.h" "c.h"
got: "a.h" "c.h"

The problem as I see is that the second elements does not include quotes, so 
it's not matched. When I change the test to read:

assert.result a.h b.h c.h 
: transform <a.h> \"b.h\" <c.h> : <([^>]*)>|\"([^\"]*)\" : 1 2 ;

I get:

expected: "a.h" "b.h" "c.h"
got: "a.h" "" "b.h" "c.h"

Since when MATCH suceeds on the second input, it returns a string of two 
elements -- first is empty string (since first group did not match anything), 
and second is "b.h". For now, I'll make the test read:

assert.result a.h "" b.h c.h 
: transform <a.h> \"b.h\" <c.h> : <([^>]*)>|\"([^\"]*)\" : 1 2 ;

But probably we need to exclude empty matches.

> > 2. Why do you need to replace all double backslashes with signle ones?
> > I'm sure you have a reason, I'd just like it to be present as comment
>
> Icons and other includes may referenced as
>
> IDR_MAINFRAME ICON "res\\icon.ico"
>
> so we have to replace double backslashes to single ones. I added this
> comment to msvc.jam

Thanks.

> > 3. The 'process' rule ends with:
> >
> > scanner.propagate c-scanner : $(angle) $(quoted) : $(target) ;
> >
> > Actually, I don't know much about RC files, so don't know if this
> > correct: do you really need c-scanner. I realize we can include header
> > which can include another header. I wonder if RC files can include other
> > *RC* files?
>
> Indeed. An RC file may include other RC files. So I changed this line:
>
> # Just propagate current scanner to includes, in a hope
> # that includes do not change scanners.
> scanner.propagate $(__name__) : $(angle) $(quoted) : $(target) ;
>
> Just like in 'c-scanner'.

Thanks again.

> > Other than this issues, I think you patch can be comitted. Would you be
> > willing to send a corrected version?
>
> See attachment. :-)

I've comitted your patch! Thanks!

- Volodya