jeffreykegler / libmarpa

Marpa parse engine C library -- STABLE
MIT License
97 stars 11 forks source link

Not reproducing trivial1.c behavior. #101

Closed dabrahams closed 2 years ago

dabrahams commented 2 years ago

Against the latest master branch, in my reproduction of trivial1.c, the assertion in this line fails, which corresponds to not getting MARPA_STEP_INACTIVE from the first call to marpa_v_step . Instead, I am getting

"symbol(Marpa.Symbol(id: 0), lhsAddress: 0, tokenValue: nil, sourceRange: Range(Marpa.EarleySet(id: 0)..<Marpa.EarleySet(id: 0)))"

which corresponds to getting MARPA_STEP_NULLING_SYMBOL.

Just before the call to marpa_v_step the valuator looks like this:

image

and afterwards, like this:

image

A second call to marpa_v_step does indeed yield MARPA_STEP_INACTIVE.

jeffreykegler commented 2 years ago

Looks like the difference is that you called marpa_g_force_valued(), as is highly recommended, since the alternative is deprecated. My trivial1.c test does not -- it tests the deprecated behavior.

One solution involves you not testing for the deprecated behavior -- that is, deleting those tests. But there's a gotcha. Turns out most of the tests in the libmarpa test suite assume the deprecated behavior. In other words, testing for the behavior I recommend (calling marpa_g_force_valued()) is the exception, and testing for the deprecated behavior is the rule. Not good.

My suggestion:

  1. I convert the tests so that most of them are for the recommended behavior.
  2. Once I decide which tests use the recommended "force valued" behavior, you delete the others. Or I may add a new test specifically for the deprecated behavior, in which case you don't have to delete any tests.

Does this plan sound OK?

By the way, I note the issue92[abc] series (recently added) are not in your test suite. They are regression tests for issue92.

jeffreykegler commented 2 years ago

@dabrahams -- the fix is to change the expectations of your test to match those of the (now fixed) trivial1 test. They can be found starting at this line:

https://github.com/jeffreykegler/libmarpa/blob/6152ae39d6d589afa82931a1c7b9d9ac3bebfbd3/test/simple/trivial1.c#L1067

Note that additional tests are added, which you also may wish to add.

The details:

I'll use FVON is designate the status of a grammar which has called marpa_g_force_valued, as recommended in my API doc. This is what you were doing but, alas, was not the general practice in my test suite. In FVON, unvalued tokens are disallowed. I recommend all downstream users use only FVON grammars.

I'll use FVOFF to is designate the status of a grammar which does not call marpa_g_force_valued. In FVOFF, unvalued tokens are allowed. FVOFF status is deprecated.

Your problem arose because you (as I recommend) tested in FVON, while the libmarpa test suite (unfortunately) tested in FVOFF.

As of https://github.com/jeffreykegler/libmarpa/commit/883328b9a4d1e5826583c7928e1e5c93cad8863a, the libmarpa test suite has added a new test depr_unvalued.c, which runs in FVOFF. All other test programs in the libmarpa test suite use FVON. This required converting many of them, including trivial1.c, from FVOFF to FVON.

I suggest that you not attempt to duplicate FVOFF tests. Specifically, I suggest that you ignore depr_unvalued.c.

The entire fix is in https://github.com/jeffreykegler/libmarpa/commit/883328b9a4d1e5826583c7928e1e5c93cad8863a, to make it convenient to see exactly what I changed.

jeffreykegler commented 2 years ago

@dabrahams -- https://github.com/jeffreykegler/libmarpa/issues/101#issuecomment-1100891535 contains my fix. Please let me know how it works out.

jeffreykegler commented 2 years ago

@dabrahams I have not heard re https://github.com/jeffreykegler/libmarpa/issues/101#issuecomment-1100892885 and will assume that is good news. I am marking this issue "about to be closed". Absent feedback to the contrary, I will close it in a couple of days.

dabrahams commented 2 years ago

Yeah, I won't have time to mess with MARPA for a week at least, possibly 2, so feel I won't be upset if you assume this is fixed and move on.

jeffreykegler commented 2 years ago

Closed as per https://github.com/jeffreykegler/libmarpa/issues/101#issuecomment-1108516053 and https://github.com/jeffreykegler/libmarpa/issues/101#issuecomment-1108658813.