search

commaai / panda

code powering the comma.ai panda
MIT License
1.53k stars 769 forks source link

Complete MISRA coverage #1954

Closed adeebshihadeh closed 2 months ago

adeebshihadeh commented 4 months ago

We've got six suppressions left, and all the remaining ones require thought on how to do cleanly and properly.

# all of the below suppressions are from new checks introduced after updating
# cppcheck from 2.5 -> 2.13. they are listed here to separate the update from
# fixing the violations and all are intended to be removed soon after
misra-c2012-1.2   # this is from the extensions (e.g. __typeof__) used in the MIN, MAX, ABS, and CLAMP macros
misra-c2012-2.5   # unused macros. a few legit, rest aren't common between F4/H7 builds. should we do this in the unusedFunction pass?
misra-c2012-8.7
misra-c2012-8.4
misra-c2012-21.15

# FIXME: violations are in ST's F4 headers
misra-c2012-12.2

Locked to @dzid26

dzid26 commented 4 months ago

Panda doesn't use typical source/header pair architecture.

Because of that rule 8.7 (reducing visibility to a given "c file") makes sense if multiple translation units are used, I would suggest its suppression. Unfortunately, cppcheck cannot detect 8.9 (presumably without global scan) if 8.7 is not enforced. 8.9 is valuable and as it enforces objects scope reduction to a given code block. More details.

I believe we have three choices:

  1. suppress 8.7 and figure how to check 8.9 without 8.7 I guess it doesn't hurt to add static to all functions in the worst case
  2. fix 8.7 and 8.9 violations even though 8.7 doesn't provide value
  3. change panda architecture to source/file pair architecture - big big refactor but would be makes sense for 8.7 (and also 8.4 but that's different story). Misra itself suggests using source/header pairs (rule 8.4).

I think we will go with 3.