search

nasa / CF

The Core Flight System (cFS) CFDP application.
Apache License 2.0
79 stars 45 forks source link

CF has strange loop construct in CF_CFDP_CycleTx #60

Closed jphickey closed 2 years ago

jphickey commented 2 years ago

This issue was imported from the GSFC issue tracking system

Imported from: [GSFCCFS-1797] CF has strange loop construct in CF_CFDP_CycleTx Originally submitted by: Hickey, Joseph P. (GSFC-582.0)[VANTAGE SYSTEMS INC] on Wed Nov 17 09:34:12 2021

Original Description: This is even noted in the comment that "code reviewers won't like this" ... which is certainly true. Not sure how this made it through review:

            goto entry_jump; / code reviewers won't like this /
            while (!args.ran_one && c->qs[CF_Q_PEND])
            {
                / didn't find anything on TXA to run, so pop one off Q_PEND and try again.
                  Keep going until CF_Q_PEND is empty or something is run /
                transaction_t t = container_of(c->qs[CF_Q_PEND], transaction_t, cl_node);
                cf_move_transaction(t, CF_Q_TXA);
                / args is ok, still { c, 0 } /
            entry_jump:
                CF_CList_Traverse(c->qs[CF_Q_TXA], CF_CFDP_CycleTx_, &args);
            }

Using a goto like this is somewhat dangerous as it goes from outside to inside a loop. Recommend to restructure the loop to use a more typical "break" statement.

jphickey commented 2 years ago

I think this one is worth fixing, just because its so outrageous. Nobody ever expects to see something like that when reading code, and its rather jarring.