Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CPPCheck Team Bravo Set 50 #38716

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

CPPCheck Team Bravo Set 50 #38716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e37ebef
Fix missingOverride where destructor was in derived class
Despiix Jan 28, 2025
bcae379
Fix cstyleCast in ADARAPackets.h
Despiix Jan 28, 2025
ee10d34
Fix TCPEventStreamDefs.h and IKafkaStreamDecoder.h
Despiix Jan 28, 2025
f5b9032
Fix knownConditionTrueFalse in ADARAPackets.cpp and unsignedPositive …
Despiix Jan 28, 2025
9e743d2
Add ExtractString and ExtractDouble methods to reduce redundant code.
Despiix Feb 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 39 additions & 32 deletions Framework/LiveData/inc/MantidLiveData/ADARA/ADARAPackets.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,7 +385,7 @@ class MANTID_LIVEDATA_DLL AnnotationPkt : public Packet {
uint32_t scanIndex() const { return m_fields[1]; }
const std::string &comment() const {
if (!m_comment.length() && (m_fields[0] & 0xffff)) {
m_comment.assign((const char *)&m_fields[2], m_fields[0] & 0xffff);
m_comment.assign(reinterpret_cast<const char *>(&m_fields[2]), m_fields[0] & 0xffff);
}

return m_comment;
Expand Down Expand Up @@ -483,13 +483,19 @@ class MANTID_LIVEDATA_DLL BeamMonitorConfigPkt : public Packet {
return (0);
}

double distance(uint32_t index) const {
if (index < beamMonCount())
return *reinterpret_cast<const double *>(&m_fields[(index * 6) + 5]);
else
return (0.0);
private:
double extractDouble(uint32_t index, uint32_t fieldOffset) const {
if (index < beamMonCount()) {
double value;
std::memcpy(&value, &m_fields[(index * 6) + fieldOffset], sizeof(double));
return value;
} else {
return 0.0;
}
}

double distance(uint32_t index) const { return extractDouble(index, 5); }

private:
const uint32_t *m_fields;

Expand All @@ -511,6 +517,23 @@ class MANTID_LIVEDATA_DLL DetectorBankSetsPkt : public Packet {
// Throttle Suffix, alphanumeric, no spaces/punctuation...
static const size_t THROTTLE_SUFFIX_SIZE = 16;

private:
std::string extractString(uint32_t index, uint32_t fieldOffset, size_t size) const {
const char *start = reinterpret_cast<const char *>(&m_fields[fieldOffset]);
return std::string(start, size);
}

private:
double extractDouble(uint32_t index, uint32_t fieldOffset) const {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if (index < detBankSetCount()) {
double value;
std::memcpy(&value, &m_fields[fieldOffset], sizeof(double));
return value;
} else {
return 0.0;
}
}

enum Flags {
EVENT_FORMAT = 0x0001,
HISTO_FORMAT = 0x0002,
Expand All @@ -526,14 +549,8 @@ class MANTID_LIVEDATA_DLL DetectorBankSetsPkt : public Packet {
}

std::string name(uint32_t index) const {
if (index < detBankSetCount()) {
char name_c[SET_NAME_SIZE + 1]; // give them an inch...
memset((void *)name_c, '\0', SET_NAME_SIZE + 1);
strncpy(name_c, (const char *)&(m_fields[m_sectionOffsets[index]]), SET_NAME_SIZE);
return (std::string(name_c));
} else {
return ("<Out Of Range!>");
}
return (index < detBankSetCount()) ? extractString(index, m_sectionOffsets[index], SET_NAME_SIZE)
: "<Out Of Range!>";
}

uint32_t flags(uint32_t index) const {
Expand Down Expand Up @@ -580,25 +597,11 @@ class MANTID_LIVEDATA_DLL DetectorBankSetsPkt : public Packet {
return (0);
}

double throttle(uint32_t index) const {
if (index < detBankSetCount()) {
return *reinterpret_cast<const double *>(&m_fields[m_after_banks_offset[index] + 3]);
} else
return (0.0);
}
double throttle(uint32_t index) const { return extractDouble(index, m_after_banks_offset[index] + 3); }

std::string suffix(uint32_t index) const {
if (index < detBankSetCount()) {
char suffix_c[THROTTLE_SUFFIX_SIZE + 1]; // give them an inch
memset((void *)suffix_c, '\0', THROTTLE_SUFFIX_SIZE + 1);
strncpy(suffix_c, (const char *)&(m_fields[m_after_banks_offset[index] + 5]), THROTTLE_SUFFIX_SIZE);
return (std::string(suffix_c));
} else {
std::stringstream ss;
ss << "out-of-range-";
ss << index;
return (ss.str());
}
return (index < detBankSetCount()) ? extractString(index, m_after_banks_offset[index] + 5, THROTTLE_SUFFIX_SIZE)
: "out-of-range-" + std::to_string(index);
}

private:
Expand Down Expand Up @@ -676,7 +679,11 @@ class MANTID_LIVEDATA_DLL VariableDoublePkt : public Packet {
uint32_t varId() const { return m_fields[1]; }
VariableStatus::Enum status() const { return static_cast<VariableStatus::Enum>(m_fields[2] >> 16); }
VariableSeverity::Enum severity() const { return static_cast<VariableSeverity::Enum>(m_fields[2] & 0xffff); }
double value() const { return *reinterpret_cast<const double *>(&m_fields[3]); }
double value() const {
double result;
std::memcpy(&result, &m_fields[3], sizeof(double));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be using the new extractDouble function you're adding.

return result;
}

void remapDeviceId(uint32_t dev) {
uint32_t *fields = reinterpret_cast<uint32_t *>(const_cast<uint8_t *>(payload()));
Expand Down
9 changes: 2 additions & 7 deletions Framework/LiveData/inc/MantidLiveData/ISIS/TCPEventStreamDefs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,21 +57,16 @@ struct TCPStreamEventHeader {
GNU_DIAG_OFF("tautological-compare")
bool isValid() const {
return marker1 == marker && marker2 == marker && length >= sizeof(TCPStreamEventHeader) &&
majorVersion() == TCPStreamEventHeader::major_version &&
minorVersion() >= TCPStreamEventHeader::minor_version && type != InvalidStream;
majorVersion() == TCPStreamEventHeader::major_version;
}
GNU_DIAG_ON("tautological-compare")

static const uint32_t major_version = 1; ///< starts at 1, then incremented whenever layout of this or further
/// packets changes in a non backward compatible way
static const uint32_t minor_version = 0; ///< reset to 0 in major version change, then incremented whenever
/// 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/// whenever layout of this or
/// further packets changes
uint32_t majorVersion() const { return version >> 16; }
uint32_t minorVersion() const { return version & 0xffff; }
};

/// header for initial data packet send on initial connection and on a state
Expand Down
2 changes: 1 addition & 1 deletion Framework/LiveData/inc/MantidLiveData/Kafka/IKafkaStreamDecoder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class MANTID_LIVEDATA_DLL IKafkaStreamDecoder {
///@{
bool isCapturing() const noexcept { return m_capturing; }
virtual bool hasData() const noexcept = 0;
std::string runId() const noexcept { return m_runId; }
const std::string &runId() const noexcept { return m_runId; }
int runNumber() const noexcept;
virtual bool hasReachedEndOfRun() noexcept = 0;
bool dataReset();
Expand Down
2 changes: 1 addition & 1 deletion Framework/LiveData/inc/MantidLiveData/LiveDataAlgorithm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class MANTID_LIVEDATA_DLL LiveDataAlgorithm : public API::Algorithm {
std::map<std::string, std::string> validateInputs() override;

protected:
~LiveDataAlgorithm() = default;
~LiveDataAlgorithm() override = default;
void initProps();

Mantid::Types::Core::DateAndTime getStartTime() const;
Expand Down
1 change: 0 additions & 1 deletion Framework/LiveData/inc/MantidLiveData/LoadLiveData.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ namespace LiveData {
*/
class MANTID_LIVEDATA_DLL LoadLiveData : public LiveDataAlgorithm {
public:
virtual ~LoadLiveData() = default;
const std::string name() const override;
/// Summary of algorithms purpose
const std::string summary() const override {
Expand Down
12 changes: 11 additions & 1 deletion Framework/LiveData/src/ADARA/ADARAPackets.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// Institut Laue - Langevin & CSNS, Institute of High Energy Physics, CAS
// SPDX - License - Identifier: GPL - 3.0 +
#include "MantidLiveData/ADARA/ADARAPackets.h"
#include "MantidKernel/WarningSuppressions.h"

#include <cstring>

Expand Down Expand Up @@ -158,8 +159,10 @@ BankedEventPkt::BankedEventPkt(const BankedEventPkt &pkt)
const Event *BankedEventPkt::firstEvent() const {
m_curEvent = nullptr;
m_curFieldIndex = 4;

// Explanation: m_curEvent can be set to not null by calling firstEventInSource
// cppcheck-suppress knownConditionTrueFalse
while (m_curEvent == nullptr && m_curFieldIndex <= m_lastFieldIndex) {
// Start of a new source
firstEventInSource();
}

Expand Down Expand Up @@ -189,6 +192,7 @@ const Event *BankedEventPkt::nextEvent() const {
}

// If we still haven't found an event, check for more source sections
// cppcheck-suppress knownConditionTrueFalse
while (m_curEvent == nullptr && m_curFieldIndex < m_lastFieldIndex) {
firstEventInSource();
}
Expand Down Expand Up @@ -217,12 +221,18 @@ void BankedEventPkt::firstEventInSource() const {

while (m_bankNum <= m_bankCount && m_curEvent == nullptr) {
firstEventInBank();
// Explanation: Although cppcheck sees 'while (m_bankNum <= m_bankCount && m_curEvent == nullptr)'
// as redundant, 'firstEventInBank()' (a const method) updates 'm_curEvent' via mutable fields.
// This means 'm_curEvent' can change from nullptr to non-null inside the loop, so the condition
// isn't actually identical or always true.
// cppcheck-suppress identicalInnerCondition
if (m_curEvent == nullptr) {
// Increment banknum because there were no events in the bank we
// just tested
m_bankNum++;
}
}

} else // no banks in this source, skip to the next source
{
m_curFieldIndex += 4;
Expand Down
20 changes: 2 additions & 18 deletions buildconfig/CMake/CppCheck_Suppressions.txt.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,24 +838,8 @@ knownConditionTrueFalse:${CMAKE_SOURCE_DIR}/Framework/Kernel/src/TimeROI.cpp:97
constParameterPointer:${CMAKE_SOURCE_DIR}/Framework/Kernel/src/TopicInfo.cpp:38
constParameterReference:${CMAKE_SOURCE_DIR}/Framework/Kernel/src/Unit.cpp:154
constParameterReference:${CMAKE_SOURCE_DIR}/Framework/Kernel/src/Unit.cpp:184
cstyleCast:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ADARA/ADARAPackets.h:388
invalidPointerCast:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ADARA/ADARAPackets.h:488
cstyleCast:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ADARA/ADARAPackets.h:531
cstyleCast:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ADARA/ADARAPackets.h:532
invalidPointerCast:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ADARA/ADARAPackets.h:585
cstyleCast:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ADARA/ADARAPackets.h:593
cstyleCast:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ADARA/ADARAPackets.h:594
invalidPointerCast:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ADARA/ADARAPackets.h:679
unsignedPositive:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ISIS/TCPEventStreamDefs.h:61
badBitmaskCheck:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/ISIS/TCPEventStreamDefs.h:70
returnByReference:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/Kafka/IKafkaStreamDecoder.h:82
missingOverride:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/LiveDataAlgorithm.h:38
missingOverride:${CMAKE_SOURCE_DIR}/Framework/LiveData/inc/MantidLiveData/LoadLiveData.h:24
knownConditionTrueFalse:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ADARA/ADARAPackets.cpp:161
knownConditionTrueFalse:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ADARA/ADARAPackets.cpp:182
knownConditionTrueFalse:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ADARA/ADARAPackets.cpp:184
knownConditionTrueFalse:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ADARA/ADARAPackets.cpp:192
identicalInnerCondition:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ADARA/ADARAPackets.cpp:220
knownConditionTrueFalse:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ADARA/ADARAPackets.cpp:187
knownConditionTrueFalse:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ADARA/ADARAPackets.cpp:185
constParameterPointer:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ISIS/DAE/idc.cpp:85
constVariable:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ISIS/DAE/idc.cpp:91
constParameterPointer:${CMAKE_SOURCE_DIR}/Framework/LiveData/src/ISIS/DAE/idc.cpp:128
Expand Down