getSubField -> getSubFieldT to avoid potential NULL de-ref.

[mdavidsaver]

Fix a couple of instances where the return values of getSubField() is dereferenced w/o checking for NULL. Also close a logic hole in pvCopy.

@mrkraimer I only make a cursory search for the most obvious cases where getSubField() is mis-used. I strongly suspect that there are more in pvCopy.cpp which are masked by the use of utility functions.

[mdavidsaver]

Can we not get distracted by a trivial optimization? The issue is fairly widespread mis-use of getSubField(). All I did was a quick grep for calls to getSubFIeld() in pvDataCPP, pvAccessCPP, and pvaSrv. I expected to find a few instances, but not this many.

[dhickin]

Can you not just make the change and add it to the pull request branch? It is, after all, trivial.

[mdavidsaver]

Ok, in the interested of keeping the peace I've made this change. My reluctance comes from having come across a few other similarly sub-optimal bits of code in the process of checking these three modules. I don't intend to optimize them all as a precondition to merging the actual bug fixes.

[dhickin]

Throwing an exception in StandardPVField::enumerated() via getSubFieldT seems reasonable. The function shouldn't ever return a null pointer though if StandardField::enumerated is correctly implemented and the pointer isn't actually dereferenced in the function, which is why I guess there are no checks. Since there's nothing in the API that say it returns null over throwing an exception I don't see an issue in allowing the exception to be thrown here. Looking through the code base no one seems to ever check the null-ness of the pointer return by enumerated, so the exception here is an improvement in my book. (Shout ASAP if you object.)

With respect to the lines

--- a/src/copy/pvCopy.cpp
+++ b/src/copy/pvCopy.cpp
@@ -444,2 +444 @@ CopyNodePtr PVCopy::createStructureNodes(
-        PVStructurePtr requestPVStructure = static_pointer_cast<PVStructure>(
-              pvFromRequest->getSubField(fieldName));
+        PVStructurePtr requestPVStructure = pvFromRequest->getSubField<PVStructure>(fieldName);

the dynamic cast is an improvement on the static cast. Note that the non-throwing version is called and the result dereferenced without checking null-ness though. Also 2 lines down we have

    PVStructurePtr pvSubFieldOptions;
    PVFieldPtr pvField = requestPVStructure->getSubField("_options");
    if(pvField) pvSubFieldOptions = static_pointer_cast<PVStructure>(pvField);

which should just be

    PVStructurePtr pvField = requestPVStructure->getSubField<PVStructure>("_options");

There are probably more of these, so I would be happy to accept the merge and do a more thorough search myself though. I'll open up an issue and assign to me unless anyone else desperately wants it.

The calls of getSubFieldT(index) are also I think an improvement, even though, again, they shouldn't be ever return null, since the pointers currently returned by getSubField(index) aren't checked before they're dereferenced.

The current call of getSubField in PVCopy::createStructureNodes is assumed non-null (it's supplied as an argument in function calls where it's dereferenced without checking) so I would say getSubFieldT is an improvement too.

I suggest this be merged - shout now if you object.

I'll raise an issue for any other null refs.

[mdavidsaver]

There are probably more of these, so I would be happy to accept the merge and do a more thorough search myself

I'm sure there are more. Thanks for undertaking this.

I suggest this be merged

Ok, unless there are immediate objections I'll do so this evening.