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

Fix #265, change variable names to be more informative #332

Merged
merged 2 commits into from
Sep 29, 2023

Conversation

havencarlson
Copy link
Contributor

Checklist (Please check before submitting)

Describe the contribution
Fix #265, change single-letter variable names to be more descriptive

Testing performed
Unit testing

Expected behavior changes
no impact to behavior

System(s) tested on

  • OS: Ubuntu 18.04

Contributor Info - All information REQUIRED for consideration of pull request
Haven Carlson - NASA

Copy link

@github-advanced-security github-advanced-security bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CodeQL-coding-standard found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

{
/* no more messages this wakeup allowed */
c->cur = t; /* remember where we were for next time */
success = false;
chan->cur = txn; /* remember where we were for next time */

Check warning

Code scanning / CodeQL-security

Local variable address stored in non-local memory Warning

A stack address which arrived via a
parameter
may be assigned to a non-local variable.
fsw/src/cf_cfdp_sbintf.c Fixed Show fixed Hide fixed
}

/* don't need CF_CRC_Start() since taken care of by reset_cfdp() */
/*CF_CRC_Start(&t->crc);*/
/*CF_CRC_Start(&txn->crc);*/

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
@havencarlson havencarlson added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Oct 5, 2022
@dzbaker dzbaker added CCB:Approved and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Oct 6, 2022
@dzbaker
Copy link
Contributor

dzbaker commented Oct 6, 2022

CCB 6 October 2022: Approved pending merge conflict resolution

@dzbaker dzbaker added CCB:Ready Ready for discussion at the Configuration Control Board (CCB) and removed CCB:Approved labels Sep 20, 2023
@havencarlson havencarlson marked this pull request as draft September 20, 2023 19:24
fsw/src/cf_cfdp_sbintf.c Fixed Show fixed Hide fixed
@@ -106,7 +106,7 @@

if (!CF_AppData.engine.out.msg)
{
c->cur = t; /* remember where we were for next time */
chan->cur = txn; /* remember where we were for next time */

Check warning

Code scanning / CodeQL

Local variable address stored in non-local memory Warning

A stack address which arrived via a
parameter
may be assigned to a non-local variable.
@havencarlson havencarlson marked this pull request as ready for review September 21, 2023 16:36
@dzbaker dzbaker added CCB:Provisionally-Approved and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Sep 21, 2023
Copy link
Contributor

@jphickey jphickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@havencarlson -- This looked great, thanks for the effort in doing this. I added a new commit to the PR to fixup a few things I saw when reviewing. Mostly - I ran it through clang-format again to fixup some of the white space that got changed due to the difference in name length.

There were a couple other minor updates - There was one function that was still using "pt" as a transaction instead of "txn" so I made it consistent.

I just committed my updates as a second commit, please have a look at 157e04a

Copy link
Contributor

@dzbaker dzbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@dzbaker dzbaker merged commit d41d8ae into nasa:main Sep 29, 2023
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Noncompliance with the concept of informative variable names
4 participants