[Gretl-devel] Re: off-by-one bug in $system.t1 (and .t2) after var
On Fri, 20 Nov 2020, Sven Schreiber wrote:
somehow I want to believe that we discussed this before, but I
can't
find any reference to it and in any case I see the behavior in the
Nov-14th snapshot.
Consider this:
<hansl>
open denmark
var 2 LRM LRY --quiet # starts at 1974:3
eval $system.t1 # 2
eval obsnum(1974:3) # 3 (correct)
</hansl>
And the same with $system.t2. But note that this only happens after
'var', not after 'system'!
IIRC the $system bundle for the "system" command came first and was
added for VARs later. But anyway, in the "system" $system bundle the
t1 and t2 values, which are always 0-based internally, are correctly
mapped to 1-based user-space values but that's not happening for
VARs. Should be easy enough to find the difference; I'll take a
look.
Some loosely related observations on C code chunks that I looked at
right now:
- We have this VAR_set_sample function and in lib/src/system.c there is
a system_adjust_t1t2 function which basically seems to have the same aim
(and internally calls list_adjust_sample from somewhere). Maybe some
consolidation in the medium term might be reasonable.
- In the equation_system_estimate function (also in lib/src/system.c)
there is this part (lines 1604-6):
if (err) {
goto system_bailout;
}
and then after another block there is (lines 1621...):
system_bailout:
if (!err) {
sys->smpl_t1 = dset->t1;
sys->smpl_t2 = dset->t2;
if (!(sys->flags & SYSTEM_LIML1)) {
set_as_last_model(sys, GRETL_OBJ_SYS);
}
}
return err;
Now I'm not too familiar with goto in C (wasn't goto declared taboo in
the old days?) ...
By some, yes! But it's quite harmless if used with care (and if it's
unidirectional, as opposed to spaghetti-style).
but to me this looks as if first err must be non-zero to jump but
then there's the opposite check for a zero err. So isn't the whole
goto thing superfluous here and in the end just err is returned
(if non-zero), or what am I missing?
I suspect the "goto" here might be residue from some more
complicated follow-up that was present at one time. But admittedly
it's not needed any more. Simply testing for (!err) as we carry out
the next few steps will work fine.
To your point that we could just return err right away, if it's
non-zero: true in this case, but I tend to prefer continuing to a
common return point at the end of the function (while not doing
anything that is bound to fail and maybe crash given the prior error
condition). That leaves the option of putting in a single debugging
statement just before return, along the lines of
fprintf(stderr, "equation_system_estimate: returning %d\n", err);
This function does, however, contain a couple of cases of "return
early on error" near the top, so this is not a hard-and-fast rule.
One further note: a undirectional forward goto can always be
replaced by an "if (condition)", which brings an extra level of
indentation, but sometimes one's code is already indented far
enough; then goto can be handy. But this function was not a good
example of that usage.
Allin