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.

Sign up for GitHub

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

Jump to bottom

Fix #1161, #1130 Apply and check standard code format #1219

Merged

astrogeco merged 7 commits into nasa:main from astrogeco:format-checks

Mar 16, 2021

Contributor

astrogeco commented Mar 11, 2021 •

edited

Loading

Describe the contribution
Fix #1161
Fix #1130

Testing performed
See checks tab https://github.com/nasa/cFE/pull/1219/checks
Bundle (fork) test: https://github.com/astrogeco/cFS/pull/8/checks

Expected behavior changes
Adds new github actions workflow that checks format. This workflow runs on both "push" and "pull_request" for all branches so at the moment it will trigger twice for any branch that is part of a pull request

System(s) tested on
Ubuntu 18.04

Additional context
Will create a tag after merge

Third party code
None

astrogeco added a commit to astrogeco/cFS that referenced this pull request


          Add nasa/cFE#1219

3de1adb

skliper reviewed

View reviewed changes

Contributor

skliper left a comment

Many splits should be cleaned up... Stopped after identifying 10

modules/core_api/fsw/inc/cfe_es.h Outdated

-              **                          \arg #CFE_PSP_RST_TYPE_PROCESSOR   - Attempts to retain volatile disk, critical data store and user reserved memory.
+              **                          \arg #CFE_PSP_RST_TYPE_POWERON     - Causes all memory to be cleared
+              **                          \arg #CFE_PSP_RST_TYPE_PROCESSOR   - Attempts to retain volatile disk, critical data store
+              *and user reserved memory.

Contributor

skliper Mar 11, 2021

Fix line split

modules/core_api/fsw/inc/cfe_es.h Outdated

Comment on lines 495 to 498

+              **                                 The caller can set this pointer to NULL if the Sub-Type is of no interest. \n
+              **ResetSubtypePtr If the provided pointer was not \c NULL, the Reset Sub-Type is stored at the given address.
+              **                                 For a list of possible Sub-Type values, see \link #CFE_PSP_RST_SUBTYPE_POWER_CYCLE
+              *"Reset Sub-Types" \endlink.

Contributor

skliper Mar 11, 2021

Fix line splits

modules/core_api/fsw/inc/cfe_es.h Outdated

Comment on lines 706 to 709

+              ** \param[out]   TaskInfo     Pointer to a \c CFE_ES_TaskInfo_t structure that holds the specific
+              **                            task information. *TaskInfo is the filled out \c CFE_ES_TaskInfo_t structure containing
+              *the
               **                            Task Name, Parent App Name, Parent App ID among other fields.

Contributor

skliper Mar 11, 2021

Fix line split

modules/core_api/fsw/inc/cfe_es.h Outdated

Comment on lines 827 to 828

		** \param[in, out] TaskIdPtr A pointer to a variable that will be filled in with the new task's ID. *TaskIdPtr is
		*the Task ID of the newly created child task.

Contributor

skliper Mar 11, 2021

Fix line split

modules/core_api/fsw/inc/cfe_es.h

		** \param[in] TaskId The task ID previously obtained when the Child Task was created with the
		*#CFE_ES_CreateChildTask API.

Contributor

skliper Mar 11, 2021

Fix line split

modules/core_api/fsw/inc/cfe_es.h Outdated

Comment on lines 1075 to 1076

		** \param[in, out] HandlePtr Pointer Application's variable that will contain the CDS Memory Block Handle.
		**HandlePtr is the handle of the CDS block that can be used in

Contributor

skliper Mar 11, 2021

Fix line split

modules/core_api/fsw/inc/cfe_es.h Outdated

Comment on lines 1195 to 1196

		** \param[in, out] RestoreToMemory A Pointer to the block of memory that is to be restored with the contents of the
		CDS. RestoreToMemory is the contents of the specified CDS.

Contributor

skliper Mar 11, 2021

Fix line split

modules/core_api/fsw/inc/cfe_es.h Outdated

Comment on lines 1226 to 1227

		** \param[in, out] PoolID A pointer to the variable the caller wishes to have the memory pool handle kept in.
		**PoolID is the memory pool handle.

Contributor

skliper Mar 11, 2021

Fix line split

modules/core_api/fsw/inc/cfe_es.h Outdated

Comment on lines 1285 to 1286

		** \param[in, out] PoolID A pointer to the variable the caller wishes to have the memory pool handle kept in.
		**PoolID is the memory pool handle.

Contributor

skliper Mar 11, 2021

Fix line split

modules/core_api/fsw/inc/cfe_es.h Outdated

Comment on lines 1291 to 1303

+              ** \param[in]   Size           The size of the pool of memory.  Note that this must be an integral number of 32 bit
+              *words.
               **
               ** \param[in]   NumBlockSizes  The number of different block sizes specified in the \c BlockSizes array. If set equal to
               **                             zero or if greater than 17, then default block sizes are used.
               **
               ** \param[in]   BlockSizes     Pointer to an array of sizes to be used instead of the default block sizes specified by
-              **                             #CFE_PLATFORM_ES_MEM_BLOCK_SIZE_01 through #CFE_PLATFORM_ES_MAX_BLOCK_SIZE.  If the pointer is equal to NULL,
+              **                             #CFE_PLATFORM_ES_MEM_BLOCK_SIZE_01 through #CFE_PLATFORM_ES_MAX_BLOCK_SIZE.  If the
+              *pointer is equal to NULL,
               **                             the default block sizes are used.
               **
-              ** \param[in]   UseMutex       Flag indicating whether the new memory pool will be processing with mutex handling or not.
+              ** \param[in]   UseMutex       Flag indicating whether the new memory pool will be processing with mutex handling or
+              *not.

Contributor

skliper Mar 11, 2021

Fix line splits

Contributor

skliper commented Mar 11, 2021

Note - could just merge as-is and clean up in follow-on. This is all due to the original having violated the 120 character rule, so it was bad then and it's bad now. The critical point is to get the bulk format in since in general it's a huge consistency improvement.

Contributor Author

astrogeco commented Mar 11, 2021 •

edited

Loading

Note - could just merge as-is and clean up in follow-on. This is all due to the original having violated the 120 character rule, so it was bad then and it's bad now. The critical point is to get the bulk format in since in general it's a huge consistency improvement.

Is the problem is mainly ensuring the two ** and a space after them?

So we'd like this "bad" example

** \param[in, out]   PoolID      A pointer to the variable the caller wishes to have the memory pool handle kept in.
**PoolID is the memory pool handle.

to look like this (at the very least)

** \param[in, out]   PoolID      A pointer to the variable the caller wishes to have the memory pool handle kept in.
** PoolID is the memory pool handle.

or in a perfect world look like

** \param[in, out]   PoolID      A pointer to the variable the caller wishes to have the memory pool handle kept in.
**                               PoolID is the memory pool handle.

Contributor

jphickey commented Mar 11, 2021

This is where I recommend (and have recommended in the past) going to a single "*" char rather than "**" on the block comments. I think most tools/auto-formatters work better with this pattern, the double * is a little unusual.... (Might be possible to fix this with a sed script)

Contributor Author

astrogeco commented Mar 11, 2021

What's the history and reasoning behind the ** style?

Contributor

jphickey commented Mar 11, 2021

No reason AFAIK, someone used it at one time, and others copied. In most IDEs I've used it defaults to just a single *, and I think in this case its confusing clang-format and causing the weird breaks you are seeing.

Contributor

skliper commented Mar 11, 2021

The GSFC coding standard uses ** in all the examples. The development guide also.

Contributor Author

astrogeco commented Mar 11, 2021

That coding standard keeps haunting us!

Contributor

jphickey commented Mar 11, 2021

Is it really part of the coding standard or did they just happen to use ** in the examples?

There's a difference - in a standards document unless they use "shall"/"must" statements - then they are just examples, not requirements. As a contributing author to some standards document, its frustrating when readers misinterpret examples as requirements.

Either way - its a case where we should really consider weighing compatibility with current industry standards vs. compatibility with some examples in a document written many years ago that may not have a real reason behind it other than being what was frequently used in code in that organization at that time (which itself I assume was nothing more than the personal preference of some original authors).

Not that I have a strong preference - from my POV it is simply a question of how much time will be spent manually fixing up the statements - neither Eclipse nor VS Code nor clang-format seem to work well with ** comment blocks, requiring manual intervention to fix them all the time, yet they all generally work OK with auto-formatting the * comment blocks and require no manual fixups.

We can certainly keep the ** - but it will cost more in terms of time and effort, vs. using the industry standard, that's all.

Contributor Author

astrogeco commented Mar 11, 2021

Random question, how does doxygen deal with the **?

Contributor

jphickey commented Mar 11, 2021

Doxygen seems generally OK with it, as long as the opening comment block starts with the correct flag (e.g. /**) then it seems to just ignore whatever chars are repeating on the left side of every line of the block.

astrogeco force-pushed the format-checks branch 3 times, most recently from d30accd to 135f52a Compare

March 11, 2021 23:33

astrogeco marked this pull request as ready for review

March 12, 2021 14:50

Contributor

skliper commented Mar 12, 2021

Is it really part of the coding standard or did they just happen to use ** in the examples?

There is no requirement to use the ** style. I'm definitely in favor of the more tool-friendly comment style, *.

astrogeco force-pushed the format-checks branch from 135f52a to 9f7a28f Compare

March 12, 2021 15:14

Contributor

jphickey commented Mar 15, 2021

It would be nice to also check for superfluous whitespace in the CMakeLists.txt files too - these currently have many lines with trailing whitespaces. A simple sed/awk script applied to these files can strip this out.

astrogeco force-pushed the format-checks branch from e3bf7dc to 4213edd Compare

March 15, 2021 21:01

Contributor Author

astrogeco commented Mar 15, 2021

It would be nice to also check for superfluous whitespace in the CMakeLists.txt files too - these currently have many lines with trailing whitespaces. A simple sed/awk script applied to these files can strip this out.

Did a check in a bunch of files while I was at it.

astrogeco force-pushed the format-checks branch from 2a5cb10 to 6c5d305 Compare

March 15, 2021 22:26

astrogeco added 2 commits

March 15, 2021 18:41


          Fix nasa#1130, Apply standard format

fb78359

Applies standard format using the .clang-format configuration from the cFS bundle
Hand edits some files to account for comment splits


          Fix nasa#1161, create format-check GitHub Actions workflow

40b48f5

Adds a continuous integration check to ensure new commits adhere to standard format.
Some minor format fix

astrogeco force-pushed the format-checks branch 5 times, most recently from 48f2610 to d882a61 Compare

March 16, 2021 14:43


          Fix nasa#1130, Clean trailing whitespace: Cmake

d554b5e

Removes trailing whitespace in:

- CMakeLists.txt
- *.cmake

astrogeco added 3 commits

March 16, 2021 15:03


          Fix nasa#1130, Clean trailing whitespace: Doxygen

db0b9bb

Removes trailing whitespace in:
- *.dox
- *doxyfile.in


          Fix nasa#1130, Clean trailing whitespace: XML

dc3ab6e


          Fix nasa#1130, Clean trailing whitespace: Other

f257ae5

Excludes .md, .pdf, and .doc

astrogeco force-pushed the format-checks branch from d882a61 to f257ae5 Compare

March 16, 2021 19:05


          Bump to v6.8.0-rc1+dev41

9aee2c5

astrogeco merged commit 7f8e475 into nasa:main

astrogeco deleted the format-checks branch

April 7, 2021 18:03

skliper added this to the 7.0.0 milestone

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet