Improve compliance with public coding standards (and document non-compliance)

[skliper]

Is your feature request related to a problem? Please describe. cFS Core has been developed and maintained utilizing internal coding standards, which isn't much help to the wider community when the question of coding standards come up. Lacking documented compliance and there's a few easy fixes that could be implemented to improve compliance.

Describe the solution you'd like Could document compliance against public standards (JPL/power of 10/etc).

Some easy updates where we could improve compliance:

• () around all macros: currently not on constants
• () for precedence: currently rely on precedence rules in many cases
• a handful of elements could be file static
• a handful of elements could be local static
• side effects in expressions: there's a handful that could be expanded

Warnings we monitor and minimize occurrences:

• Conditional compilation: still have cases to support alternate configurations
• Recursion: avoided in general, carefully analyzed (current identified cases are 1 level of recursion in debug support and one other that is protected from occurring)
• Pointer type inside typedef: in general minimized (function pointers, etc)
• Complex macros: minimized (although utilized in debugging/testing)
• Definitely use function pointers (callbacks, etc), but to satisfy requirements

Areas we don't comply and debatable value:

• unchecked parameter dereferences in helper functions and "false alarms" based on status return checks (we check status return and skip logic, but don't explicitly check a provided pointer isn't NULL)
• function too long: could be a topic of future refactor but heritage/working code should trade value vs risk
• not enough assertions: style difference
• Basic Numerical Types Used: we use fixed width types where appropriate, but don't strictly disallow basic types where they make sense

Other non-compliances identified are analyzed and dispositioned (internally), examples:

• Cast Alters Value: all over in print statements to print hex (unsigned) status (signed) values
• Many false alarms for various rules (all confirmed false)
• Redundant conditions, unreachable code, useless assignments to support configurability (wouldn't be if configuration changes)

Describe alternatives you've considered None

Additional context Note - marking for discussion to trigger input. Nothing "breaking" or "critical" identified.

Requester Info Jacob Hageman - NASA/GSFC

Ping @dmknutsen

[jphickey]

When it comes to coding standards like this we need to have a specific goal. For instance- does a particular rule improve readability or does it make it more platform independent or does it help avoid future breakage? There are many "public coding standards" and many differ/vary so we can't meet all of them, and some conflict with other goals.

() around all macros: currently not on constants

Is this needed on constants? What is the justification?

() for precedence: currently rely on precedence rules in many cases

The C precedence rules are well-defined so this is not a platform/portability concern, IMO this is purely a code readability (for humans, not machines) concern. But too many () wrapping around every single item in a multi-part expression can easily go the other way and make it much less readable to humans in the end, who will have to start counting parenthesis to figure out what goes with what.

Readability is subjective at the end of the day,

a handful of elements could be file static a handful of elements could be local static

The problem with CFE code and "static" items is coverage testing. While static is great for keeping private data private and avoiding namespace collisions, it also keeps it isolated from coverage test.

side effects in expressions: there's a handful that could be expanded

This is also generally a readability concern, generally shouldn't be a downside to expanding in most cases I think.

The general concept of function pointers comes up from time to time - IMO this is not really practical to ban use of - it isn't inherently more dangerous than any other pointer (just points to code not data). I guess one could make the case that since its executed not just accessed as data that issues can be compounded if the pointer was bad, but on modern (MMU) systems a bad pointer is likely a segfault either way.

But in a modular system design like cFE we need to use this feature, modules are loaded at runtime and dynamically bound. Even just calling the entry point of a loaded module is done via function pointer. Furthermore using function pointers as callbacks from common code patterns helps re-use the code much more effectively (think iterators, generalized search routines, etc) where it can significantly reduce code duplication, which can be a huge benefit to development time/cost and stability by not having many different similar-but-different implementations around for similar tasks, just to avoid using a function pointer.

(If you can't tell I'm an advocate of function pointers, I don't know why a coding standard would deem them "evil" - they have risks but like any other pointer the benefits to modular design and code reusability can far outweigh the risks when used properly).

[astrogeco]

CCB:2021-03-31

• Start discussion.
• Used jpl because MISRA "isn't public"