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 #1474, #1552, Resolve API prototype/implementation discrepancies #1551

Merged
merged 3 commits into from
May 25, 2021

Conversation

skliper
Copy link
Contributor

@skliper skliper commented May 19, 2021

Describe the contribution
Fix #1474 - This checks prototypes against implementation and fixes any differences (return type, parameter names, etc). For the most part implementation was considered truth (fixed prototype) except for CFE_Status_t return type change. Also fixes some but not all use of CFE_Status_t in the implementations.

Fix #1552 - note completed ut-stub updates after CCB, added in since they are trival.

Follow on work needed - Internal use of CFE_Status_t (#921)

Testing performed
Built and ran unit tests, all passed. Built usersguide and confirmed no errors.

Expected behavior changes
None

System(s) tested on

  • Hardware: Docker on laptop
  • OS: Ubuntu 18.04
  • Versions: Bundle main + these commits

Additional context
Builds on commit from @zachar1a submitted in #1531, now closed as duplicate to this PR

Third party code
None

Contributor Info - All information REQUIRED for consideration of pull request
Jacob Hageman - NASA/GSFC
Zachary Gonzalez - Individual

@skliper skliper added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label May 19, 2021
@skliper skliper added this to the 7.0.0 milestone May 19, 2021
@astrogeco
Copy link
Contributor

astrogeco commented May 19, 2021

CCB:2021-05-19 APPROVED

  • Builds on community contribution
  • Removes old UT_TimeHandler
  • There's open issues for future work

@astrogeco astrogeco added the CCB:Approved Indicates code review and approval by community CCB label May 19, 2021
Changing the implementation return types to CFE_Status_t
to match the function prototypes.
@skliper skliper changed the title Partial #1474, Resolve API prototype/implementation discrepancies Fix #1474, Resolve API prototype/implementation discrepancies May 19, 2021
@skliper
Copy link
Contributor Author

skliper commented May 19, 2021

Added in the ut-stub updates with 7e3effd

@skliper skliper requested a review from jphickey May 19, 2021 19:35
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.

Note, going forward we should consider the names in the header/documentation to be part of the API. That is, a name change in the header is treated similarly to an API change like adding or removing an argument. That's because unit test hooks may now be referencing parameters by the names in the header/documentation.

So if/when in the future there are discrepancies found between header and implementation, it would be less impact (compatibility-wise) to update the implementation to match the header, rather than the header to match the implementation.

@astrogeco astrogeco changed the base branch from main to integration-candidate May 24, 2021 17:16
@astrogeco astrogeco added the community Community contribution, YAY! label May 24, 2021
@astrogeco
Copy link
Contributor

@skliper I see that the commits are all partials. Does their combination result in a full fix for both #1474 and #1552?

@astrogeco astrogeco removed the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label May 24, 2021
@zachar1a
Copy link
Contributor

@astrogeco I only changed the implementation to match the prototypes for #1474. No work was done on my part to complete #921, another issue was also mentioned to remove the extern from both prototype and implementation. I didn't know if that should be another ticket or go in with this, so I just left them there.

@skliper
Copy link
Contributor Author

skliper commented May 25, 2021

@astrogeco The full set of commits does fix those issues as described and linked in the PR.

@skliper
Copy link
Contributor Author

skliper commented May 25, 2021

@astrogeco Amended the commit message on the last one since it alone fixed #1552, the first two together fixed #1474.

@astrogeco astrogeco changed the title Fix #1474, Resolve API prototype/implementation discrepancies Fix #1474, #1552, Resolve API prototype/implementation discrepancies May 25, 2021
@astrogeco astrogeco merged commit 077bd82 into nasa:integration-candidate May 25, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request May 25, 2021
nasa/cFE#1551, fixes discrepancies (return type, parameter names, etc) between function protoypes and implementation. Updates stubs accordingly
astrogeco added a commit to nasa/cFS that referenced this pull request May 26, 2021
Combines:

- cfe v6.8.0-rc1+dev593 (nasa/cFE#1568)
- osal v5.1.0-rc1+dev458 (nasa/osal#1050)

Includes:

- nasa/cFE#1524, add printf format casts
- nasa/cFE#1520, accept "NULL" as entry point
- nasa/cfe #1549, add capability to generate multiple tables
- nasa/cFE#1551, fixes discrepancies (return type, parameter names, etc) between function protoypes and implementation. Updates stubs accordingly

- nasa/osal#1026, Add count sem timeout test
- nasa/osal#1026, defer cancellation when BSP locked
@skliper skliper deleted the fix1474_prototype-scrub branch October 22, 2021 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CCB:Approved Indicates code review and approval by community CCB community Community contribution, YAY!
Projects
None yet
4 participants