CppCheck cleanup (#975)

fallberg · tbowmo · commit 2d5404de9794 · 2017-11-07T20:37:36.000+01:00
* Add support for inline cppcheck suppressions
* Clean out all known cppcheck issues
* Terminate toll-gate on found cppcheck issues
diff --git a/.ci/static_analysis.groovy b/.ci/static_analysis.groovy
@@ -5,7 +5,7 @@ def cppCheck(config) {
 	sh """#!/bin/bash +x
 				cd ${config.repository_root}
 				echo "Doing cppcheck for AVR..."
-				find . -type f \\( -iname \\*.c -o -iname \\*.cpp -o -iname \\*.ino \\) | cppcheck -j 4 --file-list=- --enable=style,information --platform=.mystools/cppcheck/config/avr.xml --suppressions-list=.mystools/cppcheck/config/suppressions.cfg --includes-file=.mystools/cppcheck/config/includes.cfg --language=c++ --xml --xml-version=2 2> cppcheck-avr.xml
+				find . -type f \\( -iname \\*.c -o -iname \\*.cpp -o -iname \\*.ino \\) | cppcheck -j 4 --force --file-list=- --enable=style,information --platform=.mystools/cppcheck/config/avr.xml --suppressions-list=.mystools/cppcheck/config/suppressions.cfg --includes-file=.mystools/cppcheck/config/includes.cfg --language=c++ --inline-suppr --xml --xml-version=2 2> cppcheck-avr.xml
 				cppcheck-htmlreport --file="cppcheck-avr.xml" --title="cppcheck-avr" --report-dir=cppcheck-avr_cppcheck_reports --source-dir=."""
 	
 	publishHTML([allowMissing: false, alwaysLinkToLastBuild: false, keepAll: true,
@@ -29,9 +29,8 @@ def cppCheck(config) {
 				"grep -q \"<td>0</td><td>total</td>\" cppcheck-avr_cppcheck_reports/index.html || exit_code=\$?\n"+
 				"exit \$((exit_code == 0 ? 0 : 1))")
 	if (ret == 1) {
-		config.pr.setBuildStatus(config, 'SUCCESS', 'Toll gate (Code analysis - Cppcheck)', 'Issues found (but are not considered critical)', '${BUILD_URL}CppCheck_AVR/index.html')
-		//currentBuild.result = 'UNSTABLE'
-		//error 'Terminating due to Cppcheck error'
+		config.pr.setBuildStatus(config, 'ERROR', 'Toll gate (Code analysis - Cppcheck)', 'Issues found', '${BUILD_URL}CppCheck_AVR/index.html')
+		error 'Terminating due to Cppcheck error'
 	} else {
 		config.pr.setBuildStatus(config, 'SUCCESS', 'Toll gate (Code analysis - Cppcheck)', 'Pass', '')
 	}
diff --git a/.clang_complete b/.clang_complete
@@ -1,8 +1,3 @@
--IC:\Program Files (x86)\Arduino\hardware\arduino\avr\cores\arduino
--IC:\Program Files (x86)\Arduino\hardware\tools\avr\avr\include
 -Idrivers\Linux
 -Idrivers\ATSHA204
 -Icore
--DMY_SIGNING_ATSHA204
--DMY_SIGNING_SOFT
--DARDUINO_ARCH_AVR
diff --git a/.mystools/cppcheck/config/suppressions.cfg b/.mystools/cppcheck/config/suppressions.cfg
@@ -1,5 +1,4 @@
-toomanyconfigs
-missingInclude
-missingIncludeSystem
 ConfigurationNotChecked
 unmatchedSuppression
+// This suppression is because the problem is in an in-lined macro so in-line-suppressions does not appear to take effect
+unreadVariable:*/MyHwNRF5.cpp
diff --git a/.mystools/cppcheck/options.sh b/.mystools/cppcheck/options.sh
@@ -9,6 +9,7 @@ OPTIONS="--quiet                                      \
 	--library=${LIBRARY:-avr}                           \
 	--platform="${TOOLCONFIG}"/${PLATFORM:-avr.xml}     \
  	--includes-file="${TOOLCONFIG}"/includes.cfg	      \
+ 	--inline-suppr                                      \
 	--suppressions-list="${TOOLCONFIG}"/suppressions.cfg"
 
 echo $OPTIONS
diff --git a/core/MyGatewayTransportEthernet.cpp b/core/MyGatewayTransportEthernet.cpp
@@ -49,7 +49,11 @@ MyMessage _ethernetMsg;
 #define ARRAY_SIZE(x)  (sizeof(x)/sizeof(x[0]))
 
 typedef struct {
+	// Suppress the warning about unused members in this struct because it is used through a complex
+	// set of preprocessor directives
+	// cppcheck-suppress unusedStructMember
 	char string[MY_GATEWAY_MAX_RECEIVE_LENGTH];
+	// cppcheck-suppress unusedStructMember
 	uint8_t idx;
 } inputBuffer;
 
diff --git a/core/MyLeds.cpp b/core/MyLeds.cpp
@@ -58,7 +58,9 @@ void ledsProcess()
 	}
 	prevTime = hwMillis();
 
+#if defined(MY_DEFAULT_RX_LED_PIN) || defined(MY_DEFAULT_TX_LED_PIN) || defined(MY_DEFAULT_ERR_LED_PIN)
 	uint8_t state;
+#endif
 
 	// For an On/Off ratio of 4, the pattern repeated will be [on, on, on, off]
 	// until the counter becomes 0.
diff --git a/core/MyOTAFirmwareUpdate.cpp b/core/MyOTAFirmwareUpdate.cpp
@@ -128,7 +128,6 @@ bool firmwareOTAUpdateProcess(void)
 					MY_SERIALDEVICE.print(prbuf);
 				}
 				OTA_DEBUG(PSTR("\n"));
-				8
 			}
 #endif
 			_firmwareBlock--;
diff --git a/core/MyProtocolMySensors.cpp b/core/MyProtocolMySensors.cpp
@@ -59,7 +59,6 @@ bool protocolParse(MyMessage &message, char *inputString)
 			break;
 		case 5: // Variable value
 			if (command == C_STREAM) {
-				blen = 0;
 				while (*str) {
 					uint8_t val;
 					val = protocolH2i(*str++) << 4;
@@ -125,8 +124,6 @@ bool protocolMQTTParse(MyMessage &message, char* topic, uint8_t* payload, unsign
 {
 	char *str, *p;
 	uint8_t i = 0;
-	uint8_t bvalue[MAX_PAYLOAD];
-	uint8_t blen = 0;
 	uint8_t command = 0;
 	if (topic != strstr(topic, MY_MQTT_SUBSCRIBE_TOPIC_PREFIX)) {
 		// Prefix doesn't match incoming topic
@@ -175,9 +172,10 @@ bool protocolMQTTParse(MyMessage &message, char* topic, uint8_t* payload, unsign
 
 	// Add payload
 	if (command == C_STREAM) {
-		blen = 0;
-		uint8_t val;
+		uint8_t bvalue[MAX_PAYLOAD];
+		uint8_t blen = 0;
 		while (*payload) {
+			uint8_t val;
 			val = protocolH2i(*payload++) << 4;
 			val += protocolH2i(*payload++);
 			bvalue[blen] = val;
diff --git a/core/MySigning.cpp b/core/MySigning.cpp
@@ -111,6 +111,8 @@ void signerInit(void)
 #endif
 #if (defined (MY_ENCRYPTION_FEATURE) || defined (MY_SIGNING_FEATURE)) &&\
     !defined (MY_SIGNING_SIMPLE_PASSWD)
+	// Suppress this warning since it is only fixed on Linux builds and this keeps the code more tidy
+	// cppcheck-suppress knownConditionTrueFalse
 	if (!signerInternalValidatePersonalization()) {
 		SIGN_DEBUG(PSTR("!SGN:PER:TAMPERED\n"));
 #if defined(MY_SIGNING_FEATURE)
diff --git a/core/MySigningAtsha204Soft.cpp b/core/MySigningAtsha204Soft.cpp
@@ -103,10 +103,8 @@ bool signerAtsha204SoftInit(void)
 		_signing_node_serial_info[8] = getNodeId();
 	}
 #else
-	if (init_ok) {
-		hwReadConfigBlock((void*)_signing_hmac_key, (void*)EEPROM_SIGNING_SOFT_HMAC_KEY_ADDRESS, 32);
-		hwReadConfigBlock((void*)_signing_node_serial_info, (void*)EEPROM_SIGNING_SOFT_SERIAL_ADDRESS, 9);
-	}
+	hwReadConfigBlock((void*)_signing_hmac_key, (void*)EEPROM_SIGNING_SOFT_HMAC_KEY_ADDRESS, 32);
+	hwReadConfigBlock((void*)_signing_node_serial_info, (void*)EEPROM_SIGNING_SOFT_SERIAL_ADDRESS, 9);
 #endif
 	if (!memcmp(_signing_node_serial_info, reset_serial, 9)) {
 		unique_id_t uniqueID;
diff --git a/core/MyTransport.cpp b/core/MyTransport.cpp
@@ -940,6 +940,8 @@ void transportProcessMessage(void)
 
 void transportInvokeSanityCheck(void)
 {
+	// Suppress this because the function may return a variable value in some configurations
+	// cppcheck-suppress knownConditionTrueFalse
 	if (!transportSanityCheck()) {
 		TRANSPORT_DEBUG(PSTR("!TSF:SAN:FAIL\n"));	// sanity check fail
 		transportSwitchSM(stFailure);
diff --git a/drivers/AES/AES.cpp b/drivers/AES/AES.cpp
@@ -407,19 +407,19 @@ byte AES::cbc_encrypt (byte * plain, byte * cipher, int n_block)
 
 /******************************************************************************/
 
-byte AES::decrypt (byte plain [N_BLOCK], byte cipher [N_BLOCK])
+byte AES::decrypt (byte cipher [N_BLOCK], byte plain [N_BLOCK])
 {
 	if (round) {
 		byte s1 [N_BLOCK] ;
-		copy_and_key (s1, plain, (byte*) (key_sched + round * N_BLOCK)) ;
+		copy_and_key (s1, cipher, (byte*) (key_sched + round * N_BLOCK)) ;
 		inv_shift_sub_rows (s1) ;
 
 		for (byte r = round ; --r ; ) {
 			byte s2 [N_BLOCK] ;
 			copy_and_key (s2, s1, (byte*) (key_sched + r * N_BLOCK)) ;
 			inv_mix_sub_columns (s1, s2) ;
 		}
-		copy_and_key (cipher, s1, (byte*) (key_sched)) ;
+		copy_and_key (plain, s1, (byte*) (key_sched)) ;
 	} else {
 		return AES_FAILURE ;
 	}
diff --git a/drivers/ATSHA204/sha256.cpp b/drivers/ATSHA204/sha256.cpp
@@ -30,6 +30,10 @@ const uint8_t sha256InitState[] PROGMEM = {
 	0x19,0xcd,0xe0,0x5b  // H7
 };
 
+// Suppress warning about uninitialized variables because initializing them in an init function
+// allows the compiler to optimize away the variables in case the class is only instantiated but
+// never used.
+// cppcheck-suppress uninitMemberVar
 Sha256Class::Sha256Class()
 {
 	/*
@@ -165,6 +169,10 @@ uint8_t* Sha256Class::result(void)
 #define HMAC_IPAD 0x36
 #define HMAC_OPAD 0x5c
 
+// Suppress warning about uninitialized variables because initializing them in an init function
+// allows the compiler to optimize away the variables in case the class is only instantiated but
+// never used.
+// cppcheck-suppress uninitMemberVar
 HmacClass::HmacClass()
 {
 }
diff --git a/drivers/AVR/DigitalIO/PinIO.h b/drivers/AVR/DigitalIO/PinIO.h
@@ -41,6 +41,10 @@ class PinIO
 {
 public:
 	/** Create a PinIO object with no assigned pin. */
+	// Suppress warning about uninitialized variables because initializing them in an init function
+	// allows the compiler to optimize away the variables in case the class is only instantiated but
+	// never used.
+	// cppcheck-suppress uninitMemberVar
 	PinIO() : bit_(0), mask_(0XFF) {}
 	/** Constructor
 	 * @param[in] pin Pin assigned to this object.
@@ -186,4 +190,4 @@ class PinIO
 	volatile uint8_t* portReg_;
 };
 #endif  // PinIO_h
-/** @} */
+/** @} */
diff --git a/drivers/AVR/DigitalIO/examples/SoftDS1307Utility/SoftDS1307Utility.ino b/drivers/AVR/DigitalIO/examples/SoftDS1307Utility/SoftDS1307Utility.ino
@@ -179,7 +179,6 @@ void dumpAll(void)
 //------------------------------------------------------------------------------
 void fillNvRam(void)
 {
-	uint8_t buf[8];
 	PgmPrint("Enter HEX value for all NV RAM locations (00-FF): ");
 	uint16_t v;
 	if (!hexRead(&v)) {
@@ -363,4 +362,4 @@ void loop(void)
 	} else {
 		PgmPrintln("Invalid option");
 	}
-}
+}
diff --git a/drivers/AltSoftSerial/AltSoftSerial.cpp b/drivers/AltSoftSerial/AltSoftSerial.cpp
@@ -131,13 +131,14 @@ void AltSoftSerial::writeByte(uint8_t b)
 
 ISR(COMPARE_A_INTERRUPT)
 {
-	uint8_t state, byte, bit, head, tail;
+	uint8_t state, byte, head, tail;
 	uint16_t target;
 
 	state = tx_state;
 	byte = tx_byte;
 	target = GET_COMPARE_A();
 	while (state < 9) {
+		uint8_t bit;
 		target += ticks_per_bit;
 		bit = byte & 1;
 		byte >>= 1;
@@ -195,9 +196,8 @@ void AltSoftSerial::flushOutput(void)
 
 ISR(CAPTURE_INTERRUPT)
 {
-	uint8_t state, bit, head;
-	uint16_t capture, target;
-	int16_t offset;
+	uint8_t state, bit;
+	uint16_t capture;
 
 	capture = GET_INPUT_CAPTURE();
 	bit = rx_bit;
@@ -217,8 +217,10 @@ ISR(CAPTURE_INTERRUPT)
 			rx_state = 1;
 		}
 	} else {
+		uint16_t target;
 		target = rx_target;
 		while (1) {
+			int16_t offset;
 			offset = capture - target;
 			if (offset < 0) {
 				break;
@@ -227,6 +229,7 @@ ISR(CAPTURE_INTERRUPT)
 			target += ticks_per_bit;
 			state++;
 			if (state >= 9) {
+				uint8_t head;
 				DISABLE_INT_COMPARE_B();
 				head = rx_buffer_head + 1;
 				if (head >= RX_BUFFER_SIZE) {
diff --git a/drivers/BCM/bcm2835.c b/drivers/BCM/bcm2835.c
@@ -113,11 +113,11 @@ unsigned int bcm2835_version(void)
  */
 uint32_t bcm2835_peri_read(volatile uint32_t* paddr)
 {
-	uint32_t ret;
 	if (debug) {
 		printf("bcm2835_peri_read  paddr %08X\n", (unsigned) paddr);
 		return 0;
 	} else {
+		uint32_t ret;
 		__sync_synchronize();
 		ret = *paddr;
 		__sync_synchronize();
@@ -438,7 +438,7 @@ void bcm2835_delayMicroseconds(uint64_t micros)
 
 	if (debug) {
 		/* Cant access sytem timers in debug mode */
-		printf("bcm2835_delayMicroseconds %lld\n", micros);
+		printf("bcm2835_delayMicroseconds %" PRIu64 "\n", micros);
 		return;
 	}
 
diff --git a/drivers/Linux/Stream.cpp b/drivers/Linux/Stream.cpp
@@ -30,9 +30,9 @@
 // private method to read stream with timeout
 int Stream::timedRead()
 {
-	int c;
 	_startMillis = millis();
 	do {
+		int c;
 		c = read();
 		if(c >= 0) {
 			return c;
@@ -45,9 +45,9 @@ int Stream::timedRead()
 // private method to peek stream with timeout
 int Stream::timedPeek()
 {
-	int c;
 	_startMillis = millis();
 	do {
+		int c;
 		c = peek();
 		if(c >= 0) {
 			return c;
@@ -61,8 +61,8 @@ int Stream::timedPeek()
 // discards non-numeric characters
 int Stream::peekNextDigit()
 {
-	int c;
 	while(1) {
+		int c;
 		c = timedPeek();
 		if(c < 0) {
 			return c;  // timeout
diff --git a/drivers/PubSubClient/PubSubClient.cpp b/drivers/PubSubClient/PubSubClient.cpp
diff --git a/drivers/PubSubClient/PubSubClient.h b/drivers/PubSubClient/PubSubClient.h
diff --git a/drivers/SPIFlash/SPIFlash.cpp b/drivers/SPIFlash/SPIFlash.cpp
diff --git a/hal/transport/MyTransportRS485.cpp b/hal/transport/MyTransportRS485.cpp
diff --git a/projects/SublimeText/MySensors.sublime-project b/projects/SublimeText/MySensors.sublime-project

Original file line number	Diff line number	Diff line change
`@@ -58,7 +58,9 @@ void ledsProcess()`
`58`	`58`	`}`
`59`	`59`	`prevTime = hwMillis();`
`60`	`60`
	`61`	`+#if defined(MY_DEFAULT_RX_LED_PIN) \|\| defined(MY_DEFAULT_TX_LED_PIN) \|\| defined(MY_DEFAULT_ERR_LED_PIN)`
`61`	`62`	`uint8_t state;`
	`63`	`+#endif`
`62`	`64`
`63`	`65`	`// For an On/Off ratio of 4, the pattern repeated will be [on, on, on, off]`
`64`	`66`	`// until the counter becomes 0.`
Original file line number	Diff line number	Diff line change
`@@ -128,7 +128,6 @@ bool firmwareOTAUpdateProcess(void)`
`128`	`128`	`MY_SERIALDEVICE.print(prbuf);`
`129`	`129`	`}`
`130`	`130`	`OTA_DEBUG(PSTR("\n"));`
`131`		`- 8`
`132`	`131`	`}`
`133`	`132`	`#endif`
`134`	`133`	`_firmwareBlock--;`