"Eric M. Ludlam" <eric@...> writes:
> On 01/31/2013 07:16 PM, Stephen Leake wrote:
>> I bug report 13593 on debbugs.gnu.org;
>> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=13593
>>
>> I tried to report it to the cedet sourceforge tracker, but apparently
>> only cedet project members can file bug reports there.
>
> Hi,
>
> As far as I know, there are no restrictions on the sourceforge
> tracker, but thanks for reporting somewhere.
Hmm. I just tried again, and it now lets me submit a bug. Must have been
a gremlin of some sort.
I've copied the debbugs report; sourceforge bug 3602942. I'll add that
to the debbugs report as a reference.
> The region1, region2 notation is used in a few different places in
> existing grammars. Your patch changes the behavior unconditionally.
>
> Could you supply an example where region comes out incorrectly?
Yes, but later
> I'm not familiar enough with the internals of wisent to know the
> answer here right off, and am hesitant to make an unconditional
> change. We unfortunately don't have unit tests for this level of
> feature, only existing grammars that happen to use the feature.
I fixed the code to match the structure of the $region vars supplied by
the action code wrapper built by wisent-semantic-action in
cedet/semantic/wisent/comp.el. That does:
(let* (,@bl
($region
,(cond
((= n 1)
(if (assq '$region1 bl)
'$region1
`(cdr (aref ,stack (1- ,sp)))))
((> n 1)
`(wisent-production-bounds
,stack (- ,sp ,(1- (* 2 n))) (1- ,sp)))))
Which generates the code:
(let* (($region (cdr (aref stack (1- sp)))))
The stack built by wisent-parse looks like this:
[ ... (symbol (f l)) ... ]
Note that the region is a list, not a cons. executing the '$region' code on
this gives:
(setq stack [('symbol (10 20))])
(cdr (aref stack 0))
((10 20))
My version of wisent-production-bounds expects that stack structure, and
returns that region structure (I just realized I forgot to edit the doc
string).
I just noticed there are several other places in wisent.el that refer to
a region as (f . l), at least in the doc string (I didn't check the
actual code); I'm not using any of those functions in my grammar
actions. So perhaps the right fix is:
1) fix the parser to push (f . l) on the stack, not (f l). I haven't
looked for where this change would be.
2) fix the action code to set $region to (f . l), not ((f l)) ; ie,
change 'cdr' to 'cadr'.
However, that would break all current user action code! Any grammar
action code that uses $region with wisent-parse now must be assuming the
structure is ((f l)) because that works when the region is from a
terminal, and therefore doesn't work when the region is from a
non-terminal. That's how I noticed the bug.
The alternative is to fix the rest of the code in wisent.el to use ((f
l)) for $region. I'm not clear why this hasn't been reported before; I
must be missing something.
This region structure is independent of the grammar; it is defined by
the parser code and grammar compiler code.
I think there is only one grammar compiler (wisent-parser-automaton in
semantic/wisent/comp.el), but there are two parsers; wisent-parse in
semantic/wisent/wisent.el, and semantic-bovinate-stream in
semantic/bovine.el. I have not investigated the latter to see what the
stack structure is. If it's different from wisent-parse, that might
explain the problem.
--
-- Stephe
|