Fix compiler warning Issue #21 #22
Conversation
src/DMXSerial2.cpp
Outdated
| @@ -788,7 +788,7 @@ void DMXSerialClass2::_processRDMMessage(byte CmdClass, uint16_t Parameter, bool | |||
| WRITEINT(_rdm.packet.Data+ 6, E120_SENSOR_DEFINITION); | |||
| WRITEINT(_rdm.packet.Data+ 8, E120_SENSOR_VALUE); | |||
| } | |||
| for (int n = 0; n < _initData->additionalCommandsLength; n++) { | |||
| for (uint16_t n = 0; n < _initData->additionalCommandsLength; ++n) { | |||
There was a problem hiding this comment.
I don't know how @mathertel feels, but given the rest of the codebase uses n++, I'd say it would be good to be consistent (or change the remainder of the codebase, but in which case it should probably be in another commit.
There was a problem hiding this comment.
Hi Peter,
Sorry, as a full time C++ developer, it is automatic for me to use the pre-increment operator rather than post-increment in for loops.
For a scalar like n there is absolutely no difference, however for non-scalars, a post-increment is more expensive than pre-increment because you have to copy the whole of the object to return the "before" value.
I have just looked through the codebase and I cant see any non-scalar use of post-increment so if you prefer, I can change it back to post increment - probably tomorrow as I am supposed to be working!
There was a problem hiding this comment.
Pre/Post increment fixed in commit 396551f
There was a problem hiding this comment.
Thanks @g0uus . The same issue had obviously come up either here or elsewhere before, as I'd had the efficiency thing in my Google history.
If it's better practise, it might be worth updating it throughout, but like I say that's down to @mathertel really; my main interest was just in it being consistent throughout to make it easier for novices to understand.
There was a problem hiding this comment.
I have learned C by this book: http://www.lysator.liu.se/c/bwk-tutor.html#for. The use post-increment. I agree there is no difference so there should not be a change on this.
Switching the type is correct to avoid the warning.
Thanks for the contribution.
Removed last remaining warning in DMSSerial2 itself.