Code review comment for ~bfrk/epics-base:address-modifiers

Hi Andrew, thanks for your comments.

>> +long dbPutModifier(DBADDR *paddr, short dbrType,
>> + const void *pbuffer, long nRequest, dbAddrModifier *pmod)
>> {
>> dbCommon *precord = paddr->precord;
>> short field_type = paddr->field_type;
>> + short field_size = paddr->field_size;
>> long no_elements = paddr->no_elements;
>> long special = paddr->special;
>> void *pfieldsave = paddr->pfield;
>> rset *prset = dbGetRset(paddr);
>> long status = 0;
>> + long available_no_elements = no_elements;
>
> s/available_no_elements/capacity/ maybe? It's a lot shorter...

Ah, but it's used with the opposite meaning: while initialized with the
capacity, it gets overwritten with the /current/ number of elements by
get_array_info which was previously ignored (passing &dummy).

> Please add the license header to all new source files; the Copyright lines aren't essential, but the 2 lines starting "EPICS BASE is distributed..." should be included.

Will do.

> I could see us wanting to add more kinds of modifiers in the future, although I'm not looking to make them pluggable at run-time. (NB: I'm speculating here).

Should that allow to combine multiple address modifiers, like for
filters? This would be very hard to get right. For the moment I will
assume that only one address modifier can be given per channel.

(Run-time pluggability indeed seems excessive, given that we have to
restart IOCs anyway whenever the database gets re-configured i.e.
records added or removed).

> Could you extract the array-specific operations and data and make dbAddrModifier polymorphic, with the implementation collected into a separate source file maybe, so other modifiers could be added too?

Separating out the code I added to dbPut was already on my TODO list.
Especially after adding ad-hoc index lists as another form of modifier
as you had suggested previously, it became clear that this doesn't
scale. So I will start with this move and then see if/how I can make
address modifiers polymorphic.

> I still want to be able to create a put filter that could accept a JSON string as the value, but this implementation currently hard-codes the dbAddrModifier to only support array filters. My filter doesn't have to use JSON in the PV name modifier, but it does need to do different things when the value comes in and I don't want to have to add that processing code to the dbAccess.c file. Maybe you could even extract the code for the '$' modifier into a separate parse-time routine too?
>
> I'm trying to increase modularity here.

I understand your point. Be warned though that this will increase the
overall complexity, not sure yet how much. Also, and unfortunately,
polymorphism in C is always kind of hazardous and an invitation for
stupid mistakes that should be type errors but can't because the type
checker is too dumb (see the error I recently made with &prec->val
versus prec->val).

>> +/** @brief The address modifier that modifies nothing. */
>> +#define defaultAddrModifier (struct dbAddrModifier){0,1,-1}
>
> Not sure that all our supported C compilers can accept this syntax, but this macro isn't actually being used anywhere in your code anyway. Delete?

Yes. It was left over from a previous incarnation. Code can use NULL
pointer now.

>> plug = dbFindFilter("arr", 3);
>> if (!plug) {
>> - status = S_dbLib_fieldNotFound;
>
> Why did you remove this line? It's there to reject PV names that use this modifier when the "arr" filter hasn't been loaded by the IOC.

Because we can now interpet the array address modifier even if there is
no arr filter plugin. However, assuming there is no such filter plugin,
we currently only use that information for put operations and not for
get/monitor. This is an inconsistency, but I am not sure how to solve
it, given that at this point we have no idea what kind of operation is
planned for this channel.