Hello Alvaro,
Thanks for the review and improvements.
>> Attached v4 is a rebase after 409231919443984635b7ae9b7e2e261ab984eb1e
>
> Attached v5. I thought that separating the part that executes the
> command was an obvious readability improvement.
Hmm. It is somehow, but the aim of the refactoring is to make *ALL* state
transitions to happen in doCustom's switch (st->state) and nowhere else,
which is defeated by creating the separate function.
Although it improves readability at one level, it does not help figuring
out what happens to states, which is my primary concern: The idea is that
reading doCustom is enough to build and check the automaton, which I had
to do repeatedly while reviewing Marina's patches.
Also the added doCustom comment, which announces this property becomes
false under the refactoring function:
All state changes are performed within this function called by threadRun.
So I would suggest not to create this function.
> Do we really use the word "move" to talk about state changes? It sounds
> very odd to me. I would change that to "transition" -- would anybody
> object to that? (Not changed in v5.)
Yep. I removed the goto:-)
> On INSTR_TIME_SET_CURRENT_LAZY(), you cannot just put an "if" inside a
> macro -- consider this:
> if (foo)
> INSTR_TIME_SET_CURRENT_LAZY(bar);
> else
> something_else();
> Which "if" is the else now attached to? Now maybe the C standard has an
> answer for that (I don't know what it is), but it's hard to read and
> likely the compiler will complain anyway. I wrapped it in "do { }
> while(0)" as is customary.
Indeed, good catch.
In the attached patched, I have I think included all your changes *but*
the separate function, for the reason discussed above, and removed the
"goto".
--
Fabien.