getSubField returns "nullptr" is not helpful and does not allow chaining of methods

[ghost]

element->pvStructurePtr->getSubField<PVDouble>("value.myDouble")->put(13434.454);

Since getSubField returns nullptr instead of throwing an exception, this fails in a very bad way if typo in fieldName(server gets stuck somewhere and it spins without crashing.)

[mrkraimer]

This sounds like a good idea but needs some effort to see what breaks.. I did try making the change all tests in pvDataCPP pass. But the tests in normativeTypesCPP show many failures.

[dhickin]

This strikes me as a very bad idea.

The design decision to return a null pointer rather than throw was taken a long while back. I seem to remember Marty arguing against exceptions for efficiency reasons for servers processing requests, but it doesn't really matter why we decided this.

The point is we have large amounts of code where getSubField is called, the pointer checked and apropriate action taken if null. Throwing exceptions will break all this code. Any check of null pointers and local handling would have to replaced by try/catch statements.

Bear in mind a nullptr is not necessarily an error. For example many fields in NTs are optional and will return null if getSubField is called. We would definitely have to rewrite all this code using try/catch statements else every optional field will generate uncaught exceptions.

Bear in mind it's pretty easy to write wrapper functions or macros to throw exceptions on a null pointer, so there's no need to change the existing behaviour of getSubField. If absolutely necessary a second function, e.g. getSubFieldChecked(), could be added that throws exceptions.

We are also, I believe, 3 weeks away from feature freeze on the 4.5 release.

Dave

---

From: Marty Kraimer [notifications@github.com] Sent: 22 June 2015 15:52 To: epics-base/pvDataCPP Subject: Re: [pvDataCPP] getSubField returns "nullptr" is not helpful and does not allow chaining of methods (#2)

This sounds like a good idea but needs some effort to see what breaks.. I did try making the change all tests in pvDataCPP pass. But the tests in normativeTypesCPP show many failures.

� Reply to this email directly or view it on GitHubhttps://github.com/epics-base/pvDataCPP/issues/2#issuecomment-114139730.

This e-mail and any attachments may contain confidential, copyright and or privileged material, and are for the use of the intended addressee only. If you are not the intended addressee or an authorised recipient of the addressee please notify us of receipt by returning the e-mail and do not use, copy, retain, distribute or disclose the information in or attached to the e-mail. Any opinions expressed within this e-mail are those of the individual and not necessarily of Diamond Light Source Ltd. Diamond Light Source Ltd. cannot guarantee that this e-mail or any attachments are free from viruses and we cannot accept liability for any damage which you may sustain as a result of software viruses which may be transmitted in or with the message. Diamond Light Source Limited (company no. 4375679). Registered in England and Wales with its registered office at Diamond House, Harwell Science and Innovation Campus, Didcot, Oxfordshire, OX11 0DE, United Kingdom

[dhickin]

(Resending to include epics developers)

This strikes me as a very bad idea.

The design decision to return a null pointer rather than throw was taken a long while back. I seem to remember Marty arguing against exceptions for efficiency reasons for servers processing requests, but it doesn't really matter why we decided this.

The point is we have large amounts of code where getSubField is called, the pointer checked and apropriate action taken if null. Throwing exceptions will break all this code. Any check of null pointers and local handling would have to replaced by try/catch statements.

Bear in mind a nullptr is not necessarily an error. For example many fields in NTs are optional and will return null if getSubField is called. We would definitely have to rewrite all this code using try/catch statements else every optional field will generate uncaught exceptions.

Bear in mind it's pretty easy to write wrapper functions or macros to throw exceptions on a null pointer, so there's no need to change the existing behaviour of getSubField. If absolutely necessary a second function, e.g. getSubFieldChecked(), could be added that throws exceptions.

We are also, I believe, 3 weeks away from feature freeze on the 4.5 release.

Dave

---

From: Marty Kraimer [notifications@github.com] Sent: 22 June 2015 15:52 To: epics-base/pvDataCPP Subject: Re: [pvDataCPP] getSubField returns "nullptr" is not helpful and does not allow chaining of methods (#2)

This sounds like a good idea but needs some effort to see what breaks.. I did try making the change all tests in pvDataCPP pass. But the tests in normativeTypesCPP show many failures.

� Reply to this email directly or view it on GitHubhttps://github.com/epics-base/pvDataCPP/issues/2#issuecomment-114139730.

This e-mail and any attachments may contain confidential, copyright and or privileged material, and are for the use of the intended addressee only. If you are not the intended addressee or an authorised recipient of the addressee please notify us of receipt by returning the e-mail and do not use, copy, retain, distribute or disclose the information in or attached to the e-mail. Any opinions expressed within this e-mail are those of the individual and not necessarily of Diamond Light Source Ltd. Diamond Light Source Ltd. cannot guarantee that this e-mail or any attachments are free from viruses and we cannot accept liability for any damage which you may sustain as a result of software viruses which may be transmitted in or with the message. Diamond Light Source Limited (company no. 4375679). Registered in England and Wales with its registered office at Diamond House, Harwell Science and Innovation Campus, Didcot, Oxfordshire, OX11 0DE, United Kingdom

[dhickin]

On 06/22/2015 01:12 PM, david.hickin@diamond.ac.uk wrote: ...

The design decision to return a null pointer rather than throw was taken a long while back.

Sigh...

The point is we have large amounts of code where getSubField is called, the pointer checked and apropriate action taken if null. Throwing exceptions will break all this code.

A quick search finds a few places where not NULL is assumed. They may not be bugs. Of course, as Arman discovered, they're only one typo away.

See below

Bear in mind it's pretty easy to write wrapper functions or macros to throw exceptions on a null pointer

Wrappers are not succinct.

If absolutely necessary a second function, e.g. getSubFieldChecked(), could be added that throws exceptions.

I think this is necessary.

We are also, I believe, 3 weeks away from feature freeze on the 4.5 release.

So 3 weeks to make changes?

From pvAccessCPP

$ git grep 'getSubField.*->' src/ src/ca/caChannel.cpp: std::tr1::static_pointer_cast(va->getSubField("highAlarmLimit"))->put(data->upper_alarm_limit); src/ca/caChannel.cpp: std::tr1::static_pointer_cast(va->getSubField("highWarningLimit"))->put(data->upper_warning_limit); src/ca/caChannel.cpp: std::tr1::static_pointer_cast(va->getSubField("lowWarningLimit"))->put(data->lower_warning_limit); src/ca/caChannel.cpp: std::tr1::static_pointer_cast(va->getSubField("lowAlarmLimit"))->put(data->lower_alarm_limit); src/ca/caChannel.cpp: bitSet->set(pvStructure->getSubField("value")->getFieldOffset()); src/remote/security.cpp: m_userAndHost->getSubField("user")->put(userName); src/remote/security.cpp: m_userAndHost->getSubField("host")->put(buffer); src/server/responseHandlers.cpp: help->getSubField("value")->put(helpString); src/server/responseHandlers.cpp: result->getSubField("version")->put(ret.str()); src/server/responseHandlers.cpp: result->getSubField("implLang")->put("cpp"); src/server/responseHandlers.cpp: result->getSubField("host")->put(hostName); src/server/responseHandlers.cpp: result->getSubField("process")->put(sspid.str()); src/server/responseHandlers.cpp: result->getSubField("startTime")->put(timeText);

From the module formerly known as easyPVACPP

$ git grep 'getSubField.*->' src/ src/easyNTMultiChannel.cpp: pvStructure->getSubField("channelName")-> src/easyNTMultiChannel.cpp: pvStructure->getSubField("isConnected")-> src/easyNTMultiChannel.cpp: pvStructure->getSubField("severity")->replace( src/easyNTMultiChannel.cpp: pvStructure->getSubField("status")->replace( src/easyNTMultiChannel.cpp: pvStructure->getSubField("message")->replace(freeze(message)); src/easyNTMultiChannel.cpp: pvStructure->getSubField("secondsPastEpoch")->replace(freeze(secondsPastEpoch)); src/easyNTMultiChannel.cpp: pvStructure->getSubField("nanoseconds")->replace(freeze(nanoseconds)); src/easyNTMultiChannel.cpp: pvStructure->getSubField("userTag")->replace(freeze(userTag));

[mrkraimer]

One possibility that does not break existing code is to add an argument:

template std::tr1::shared_ptr getSubField( std::string const &fieldName, bool raiseException = false) const;

[mdavidsaver]

bool raiseException = false

This doesn't strike me as a good idea...

[dhickin]

+1

On Jun 22, 2015, at 10:12 AM, david.hickin@diamond.ac.uk wrote:

Bear in mind a nullptr is not necessarily an error. For example many fields in NTs are optional and will return null if getSubField is called.

[dhickin]

Resolution: PVStructure::getAs<>() has been renamed getSubFieldT which throws exceptions when field is not present. getSubField will still return null pointer in this case.

If an exception is preferred getSubFieldT may be called and this will allow chaining.