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 #1336, function block comments #1429

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Apr 27, 2021

Describe the contribution
Fixes #1336

Scrub all internal APIs within CFE core to make comment blocks in sources and headers more consistent.

Whenever a function is prototyped in a header, this moves the information about the function behavior to that prototype and puts it into doxygen format. The comment block on the function implementation refers the reader to the prototype - it does not duplicate the info. Exception for local helper functions which are not separately prototyped, these may have pre/post condition info in the block itself.

All functions definitions are noted whether they intend to conform to the public API (i.e. from a header in core_api) and therefore have global scope, or if they have application-internal scope, or if they have file/local scope.

Testing performed
Build and sanity check CFE, run all unit tests

Expected behavior changes
None - documentation updates only

System(s) tested on
Ubuntu 20.04

Additional context
Changes to each subsystem are put in a separate commit.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

EDIT - also fixes #1350 (deletes instead of moves, they are not implemented)

@jphickey
Copy link
Contributor Author

Note that in cases where this moved comment info/notes/documentation from the definition to the declaration, it also converted it into doxygen markup where possible. This takes the first step at putting some doxygen information on every internal API, not just public APIs. However it does not (yet) fully document all parameters and outputs/return values. This may show up in the doxygen warnings. (it was never documented, but now it might be noticed by the tools because it was converted to doxygen style markup).

@jphickey jphickey added the CCB:Ready Ready for discussion at the Configuration Control Board (CCB) label Apr 27, 2021
@skliper
Copy link
Contributor

skliper commented Apr 28, 2021

Only nit/recommendation would be not to explicitly reference the header file name the API is defined in. I'd just put "see header file for full documentation" or similar. These move around (and will move again when refactored), so easier to keep generic and explicit reference doesn't really provide that much benefit (not worth the effort to keep up-to-date).

@astrogeco astrogeco added CCB:Approved Indicates code review and approval by community CCB and removed CCB:Ready Ready for discussion at the Configuration Control Board (CCB) labels Apr 28, 2021
@astrogeco
Copy link
Contributor

astrogeco commented Apr 28, 2021

CCB:2021-04-21 APPROVED

  • all comments, some hand edits to docs
  • Is there a way to "tell at a glance" what corresponds to public APIs? -> Yes, the file naming pattern and where the header file lives
  • Missing some parameters but we will add those in a separate issue

@jphickey
Copy link
Contributor Author

Only nit/recommendation would be not to explicitly reference the header file name the API is defined in. I'd just put "see header file for full documentation" or similar. These move around (and will move again when refactored), so easier to keep generic and explicit reference doesn't really provide that much benefit (not worth the effort to keep up-to-date).

See commit a3b8e80 which replaces this to just say "See description in header file ..."

@jphickey jphickey force-pushed the fix-1336-function-comments branch from a3b8e80 to dc59e35 Compare April 29, 2021 13:31
@jphickey
Copy link
Contributor Author

Rebased to latest "main" branch to resolve conflicts.

@astrogeco astrogeco changed the base branch from main to integration-candidate May 4, 2021 00:12
@astrogeco astrogeco merged commit 2bc0cc7 into nasa:integration-candidate May 4, 2021
astrogeco added a commit to nasa/cFS that referenced this pull request May 5, 2021
nasa/cFE#1418, add ES CDS Functional test
nasa/cFE#1429, Function comment blocks
nasa/cFE#1414, Add Header Functional tests.
nasa/cFE#1415, add Current Time Functional Test
astrogeco added a commit to nasa/cFS that referenced this pull request May 5, 2021
cfe v6.8.0-rc1+dev559 and osal v5.1.0-rc1+dev417

Combines:

nasa/osal#979
nasa/cFE#1481

Includes:

nasa/osal#973, UtPrintx function
nasa/osal#976, add socket shutdown implementation

nasa/cFE#1418, add ES CDS Functional test
nasa/cFE#1429, Function comment blocks
nasa/cFE#1414, Add Header Functional tests.
nasa/cFE#1415, add Current Time Functional Test
@jphickey jphickey deleted the fix-1336-function-comments branch May 14, 2021 14:23
@skliper skliper added this to the 7.0.0 milestone Sep 24, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move perflog prototypes from cfe_es_log.h to cfe_es_perf.h Standardize/clean function description comments
3 participants