CPPCheck Team Bravo Set 50 #38716
CPPCheck Team Bravo Set 50 #38716
Conversation
Despiix
added
the
Maintenance
Despiix
force-pushed
the
cpp-check-Set-50
branch
8 times, most recently
from
f41d0ff to
0dd88b4
Compare
| @@ -67,7 +69,7 @@ struct TCPStreamEventHeader { | |||
| static const uint32_t minor_version = 0; ///< reset to 0 in major version change, then incremented whenever | |||
There was a problem hiding this comment.
I'd suggest deleting minor_version completely, since it seems to have not changed for at least 12 years, then can get rid of the suppression on line 62 as well.
Mark LoadLiveData.h as override
plus invalidPointerCasting from int to double.
TCPEventStreamDefs unsignedPositive was comparing an int to an uint32.
…in TCPEventStreamDefs.h Suppressed some because they worked correctly. Explanations are written in comments above suppressions. Co-authored-by: thomashampson <[email protected]> Co-authored-by: Jonathan Haigh <[email protected]>
Change code based on review comments Co-authored-by: James Clarke <[email protected]>
Despiix
force-pushed
the
cpp-check-Set-50
branch
from
fb1bec0 to
9e743d2
Compare
| /// layout of this or further packets changes in a backward compatible | ||
| /// way | ||
| static const uint32_t current_version = (major_version << 16) | minor_version; ///< starts at 1, then incremented | ||
| static const uint32_t current_version = (major_version << 16); ///< starts at 1, then incremented |
There was a problem hiding this comment.
minorVersion shouldn't be deleted, because that can change with version, you only need to delete the static minor_version, then the check becomes
minorVersion() >= 0 && type != InvalidStream;At the moment the type check has gone as well, but we want to maintain existing behaviour.
| } | ||
|
|
||
| private: | ||
| double extractDouble(uint32_t index, uint32_t fieldOffset) const { |
There was a problem hiding this comment.
You've got the same code in two separate functions in different classes, both called extractDouble, the idea is to have the code in one place, because otherwise you may as well just put the duplicate code inline in both places. Because the method is needed in different classes you could either add a static method to the file, or put it on the common base class, Packet, (second option sounds better to me).
| double value() const { return *reinterpret_cast<const double *>(&m_fields[3]); } | ||
| double value() const { | ||
| double result; | ||
| std::memcpy(&result, &m_fields[3], sizeof(double)); |
There was a problem hiding this comment.
This should be using the new extractDouble function you're adding.
Description of work
Summary of work
There is no associated issue.
Further detail of work
Set 50: http://172.16.111.171/suppressionSet.php?setid=50To test:
Check if all of the issues in set 50 are fixed correctly
Reviewer
Please comment on the points listed below (full description).
Your comments will be used as part of the gatekeeper process, so please comment clearly on what you have checked during your review. If changes are made to the PR during the review process then your final comment will be the most important for gatekeepers. In this comment you should make it clear why any earlier review is still valid, or confirm that all requested changes have been addressed.
Code Review
Functional Tests
Does everything look good? Mark the review as Approve. A member of
@mantidproject/gatekeeperswill take care of it.Gatekeeper
If you need to request changes to a PR then please add a comment and set the review status to "Request changes". This will stop the PR from showing up in the list for other gatekeepers.