logo separator

[mkgmap-dev] [mkgmap-svn] Commit: r1488: RuleFileReader.optimiseAndSaveBinaryOp(): Try to be symmetric.

From Marko Mäkelä marko.makela at iki.fi on Mon Jan 18 10:16:17 GMT 2010

On Mon, Jan 18, 2010 at 09:10:49AM +0000, Steve Ratcliffe wrote:
> On 18/01/10 08:13, svn commit wrote:
> > RuleFileReader.optimiseAndSaveBinaryOp(): Try to be symmetric.
> > optimiseAndSaveAndOr(): Refactored from optimiseAndSaveBinaryOp().
> 
> So you are trying to transform
> 
>   a=b & (c=d|e=f)
> 
> to
> 
>   a=b&c=d | a=b&e=f
> 
> The second.isType(OR) will not be executed because first.isType(EQUALS)
> will have matched first.

Yes, as I wrote in the other message, this comment is somewhat bogus
(it already was before the patch).  I corrected the comment in r1490.
The following would be more appropriate:

 a~b & (c=d|e=f)
to
 a~b & c=d | a~b & e=f
and further (in optimiseAndSaveBinaryOp() on and1 and and2) to
 c=d & a~b | e=f & a~b

In fact, I tested this style definition:

a~b & (c=d|e=f) { set ab=1; }

It passes with my patch and fails without the second.isType(OR) check:

Error in style: Error: (lines:58): Invalid rule file (expr &)

> Also it is not so clear that it is an optimisation as both the new rules 
> will be under the same search key and so you will end up doing pretty 
> much the same thing in either case. With the OR first they will be under 
> different search keys and so will reduce the work in some cases.

I see, there can be at most one search key per rule?  So, it would seem to
make sense to put the most selective condition first.  And I guess then
it would not make sense to move the isType(OR) checks before
EQUALS or EXISTS.

> But more importantly it is *necessary* to deal with the case where the 
> OR comes first because the first term has to be an EQUALS or EXISTS. 
> Symmetry does not apply as the situation is asymmetrical to begin with, 
> and there is no such requirement on the second term.

Does my above example convince you that the symmetry might be useful?
Or do you think that we should throw syntax error for "stupid" style
definitions?

	Marko



More information about the mkgmap-dev mailing list