search

epics-base / pvDataCPP

pvDataCPP is an EPICS V4 C++ module
https://epics-base.github.io/pvDataCPP/
Other
6 stars 16 forks source link

replace getSubField(fieldName) with template version #34

Closed mrkraimer closed 8 years ago

mrkraimer commented 8 years ago

All instances of getSubField(fieldName) have been replaced by template version. For example

-    } else if(pvStructure->getSubField("field")) {
+    } else if(pvStructure->getSubField<PVStructure>("field")) {
dhickin commented 8 years ago

Since the pull request has come from Marty's master branch and he's done a couple of merges I think accepting the request will add some confusing extra merge commits, so if we want to merge this we'll have to sort that out first. So let's not merge this.

A few comments on the changes themselves:

  1. If we're going to replace the non-template getSubField with a template, in many cases there is a better choice than getSubField<PVField>. Looking at the tests, in most of the pvRequest tests the field will be a PVStructure according to both the pvRequest doc and the current implementation, so it would make sense to use this. Similarly construction of alarms and time stamps could use PVStructure, PVLong, PVInt, PVString etc.
  2. We have a large number (currently around 200) calls of non-template getSubField in the C++ base. Replacing calls with a template call using a more appropriate derived class is a good idea. Replacing getSubField with getSubField<PVField> is less clear cut. If we're going to get rid of these calls it should be a definite policy rather than done in a piecemeal fashion.
  3. Note dynamic pointer casting a PVFieldPtr to a PVFieldPtr isn't actually a null op. This is evident from looking both at the boost code and my compiler's implementation and from performance tests.
  4. In particular, note implementing getSubField(offset) using getSubField<PVField> for subfields means that getSubField<PVField>(offset) getSubFieldT<PVField>(offset) can do multiple unnecessary dynamic pointer casts pointer casts, especially if the field is at one or more levels below in the substructure.

I suggest we replace the non-template calls of getSubField() with the template for now only where there is a sensible derived type. Some of the replacements in the pull request are definite improvements.

I have some code with a suggested set of changes - a subset of Marty's changes plus some calls of getSubField<T> for a more appropriate derived type T. I'll publish these.

dhickin commented 8 years ago

I've published the suggested changes as #39 .

dhickin commented 8 years ago

Closed. Replace with #39