Skip to content

Fix Clang 15 warnings #81

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix Clang 15 warnings #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dbc06c1
ScalarAlarmSupport::AlarmRange now declares an enum
anjohnson Dec 25, 2023
035b285
API change to PVSupport, likely unused though
anjohnson Dec 25, 2023
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
10 changes: 10 additions & 0 deletions documentation/RELEASE_NOTES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,16 @@

This document summarizes the changes to the module between releases.

## Release 4.7.2 (UNRELEASED)

* The virtual `init()` method of `class PVSupport` has been made un-virtual, and the base-class
implementation always returns `false` as it should not be used. The `ControlSupport` and
`ScalarAlarmSupport` derived classes already provided their own implementations but needed
different method arguments, so a virtual base-class method couldn't be used anyway.
Those derived classes don't appear to be used internally at all, although they were being
used in Marty Kraimer's exampleCPP repository which is no longer maintained or recommended
for use anyway.

## Release 4.7.1 (EPICS 7.0.8, Dec 2023)

* Added data distributor plugin which can be used for distributing data between
Expand Down
5 changes: 2 additions & 3 deletions src/pv/controlSupport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,18 @@ class epicsShareClass ControlSupport :
*/
virtual ~ControlSupport();
/**
* @brief Connects to contol fields.
* @brief Connects to control fields.
*
* @param pvValue The field to support.
* @param pvSupport Support specific fields.
* @return <b>true</b> for success and <b>false</b> for failure.
*/
virtual bool init(
bool init(
epics::pvData::PVFieldPtr const & pvValue,
epics::pvData::PVFieldPtr const & pvSupport);
/**
* @brief Honors control fields.
*
*
* @return Returns true is any fields were modified; otherwise false.
*/
virtual bool process();
Expand Down
14 changes: 7 additions & 7 deletions src/pv/pvSupport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,17 +38,17 @@ class epicsShareClass PVSupport
*/
virtual ~PVSupport(){}
/**
* @brief Optional initialization method.
* @brief Required initialization method.
*
* Called after PVRecord is created but before record is installed into PVDatabase.
* Implementation classes must define an init() method that must be
* explicitly called by PVRecord classes that use them to provide
* references to the specific fields to be supported. Different support
* will may different fields, so a virtual method cannot be defined in
* this base class to support them, hence this method always fails.
*
* @param pvValue The field to support.
* @param pvSupport Support specific fields.
* @return <b>true</b> for success and <b>false</b> for failure.
*/
virtual bool init(
epics::pvData::PVFieldPtr const & pvValue,
epics::pvData::PVFieldPtr const & pvSupport) {return true;}
bool init() {return false;}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am unsure what the original intent was here, but if all classes that inherit from PVSupport were supposed to have init(pvValue, pvSupport), I do not see a problem with leaving this (and ControlSupport class) as it was, and only changing/renaming init() method on line 48 in ScalarAlarmSupport, which would presumably fix the warnings, and would be a smaller API change. I do not quite see a point of introducing a new init() method here that is not being used. Sorry if I am missing something...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think Marty was suggesting that classes derived from PVSupport would provide their own init() method to set up the their added member data, as his class ScalarAlarmSupport (in src/pv/scalarAlarmSupport.{h,cpp}) does. His Doxygen comment said the method would be optional (line 41 above) implying that some might not need to, but I think that in most cases the derived class would need additional parameters anyway (as class ScalarAlarmSupport does), so it doesn't actually make sense for init() to be a virtual method with a fixed set of parameters anyway.

My change was written to try and keep what I thought his design intent was. If we don't care about that then I agree that these changes to the base class are unnecessary.

/**
* @brief Optional method for derived class.
*
Expand Down
11 changes: 5 additions & 6 deletions src/pv/scalarAlarmSupport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,14 @@ class epicsShareClass ScalarAlarmSupport :
* @param pvSupport Support specific fields.
* @return <b>true</b> for success and <b>false</b> for failure.
*/
virtual bool init(
bool init(
epics::pvData::PVFieldPtr const & pvValue,
epics::pvData::PVStructurePtr const & pvAlarm,
epics::pvData::PVFieldPtr const & pvSupport);
/**
* @brief Honors scalarAlarm fields.
*
*
* @return Returns true is any fields were modified; otherwise false.
* @return true if any fields were modified, otherwise false.
*/
virtual bool process();
/**
Expand All @@ -77,20 +76,20 @@ class epicsShareClass ScalarAlarmSupport :
private:

ScalarAlarmSupport(PVRecordPtr const & pvRecord);
enum {
enum AlarmRange {
range_Lolo = 0,
range_Low,
range_Normal,
range_High,
range_Hihi,
range_Invalid,
range_Undefined
} AlarmRange;
};
void setAlarm(
epics::pvData::PVStructurePtr const & pvAlarm,
int alarmRange);
PVRecordPtr pvRecord;
int prevAlarmRange;
enum AlarmRange prevAlarmRange;
epics::pvData::PVScalarPtr pvValue;
epics::pvData::PVStructurePtr pvAlarm;
epics::pvData::PVStructurePtr pvScalarAlarm;
Expand Down
2 changes: 1 addition & 1 deletion src/support/scalarAlarmSupport.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ bool ScalarAlarmSupport::process()
double highWarningLimit = pvHighWarningLimit->get();
double highAlarmLimit = pvHighAlarmLimit->get();
double hysteresis = pvHysteresis->get();
int alarmRange = range_Normal;
enum AlarmRange alarmRange = range_Normal;
if(highAlarmLimit>lowAlarmLimit) {
if(value>=highAlarmLimit
||(prevAlarmRange==range_Hihi && value>=highAlarmLimit-hysteresis)) {
Expand Down