Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CF has strange loop construct in CF_CFDP_CycleTx #60

Closed
jphickey opened this issue Nov 23, 2021 · 1 comment · Fixed by #165
Closed

CF has strange loop construct in CF_CFDP_CycleTx #60

jphickey opened this issue Nov 23, 2021 · 1 comment · Fixed by #165
Assignees
Milestone

Comments

@jphickey
Copy link
Contributor

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 jphickey self-assigned this Jan 6, 2022
@jphickey
Copy link
Contributor Author

jphickey commented Jan 6, 2022

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.

jphickey added a commit to jphickey/CF that referenced this issue Jan 12, 2022
jphickey added a commit to jphickey/CF that referenced this issue Jan 12, 2022
Modifies the loop inside this function to be more conventional, and
easier to follow. Does not change the logic.
astrogeco added a commit that referenced this issue Jan 12, 2022
Fix #60, rework loop in CF_CFDP_CycleTx
@skliper skliper added this to the Draco milestone Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants