Re: [Simulavr-devel] [patch #7063] local variables' fixes

[Petr Hluzín]

On 17 February 2010 21:51, Onno Kortmann <address@hidden> wrote:
> Hi,
>> +Initialization of SystemClock::Step() local variable.
>> Do not discard the only simulation member.
>
> this patch, although short, seems to involve quite a bit of twisted logic.

Agreed. I have even had time to prepare this reply.

I think that once Step() returns the nextStepIn_ns contains duration
if the just-executed instruction.
Then (on next assignment) its meaning changes to "time since start of
simulation when the core should be scheduled again".

I did want minimize the changes. Perhaps I should have introduced
another variable.

> So just to make sure that I understand you correctly:
> First, the patch fixes the unitialized _ns clock offset
> Second, for devices requiring clock ticks one after each other (ns)
> resolution, your patch correctly handles the device that has just been
> removed in the line
>
> erase(begin());
>
> , is this correct?

Yes, kinda. If the Step() did not initialize the variable the result
was undefined. Now it is defined though still possibly wrong - e.g. I
have not found who is responsible for deleting the core.

Notice the erase-insert cycle for each instruction. Getting rid of
that should speed things up.
But code during Step() might not like it.

> Looking at the souce, I would further propose that *Freq gets renamed to
> *Period wherever it makes sense, as AvrDevice::SetClockFreq is quite
> confusing IMO.

Yes. That's a lot of lines. Do other people mind the possible conflicts?

-- 
Best Regards
Petr Hluzin