Experimental Physics and
Industrial Control System

Ben Franksen via Core-talk <core-talk at aps.anl.gov> · Wed, 01 Apr 2020 08:06:41 -0000

Am 31.03.20 um 22:17 schrieb Andrew Johnson:
> I haven't tried building or using this, but overall it looks reasonable. Unfortunately I don't think we can afford to change the prototype of dbGetField() like this because it is a published API that other modules call, but AFAIK they only ever pass NULL for the final argument – see my diff comment below for suggestions.

I offered this particular patch (commit 7287a1f) as kind of "for
discussion", as I wasn't sure how much external code still uses this
API. I only saw that the record types and device supports in base no
longer call dbGetField directly. BTW, the void *pflin member must have
been added at some point in time (it wasn't there in 3.14) and that must
have taken its toll on external modules, too. But that was a "major"
version jump so perhaps the rules allowed that.

Anyway, I can easily remove this particular patch. It is pretty much
independent from anything else I did or plan to do.

On another note, my current (unfinished) attempts at adding put filters
also include the addition of a new parameter to dbPut and dbPutField.
This is a similarly breaking change, and I guess I should do something
about that before I propose that code for merging, right?

>> diff --git a/modules/database/src/ioc/db/db_field_log.h b/modules/database/src/ioc/db/db_field_log.h
>> index 1534517..a801142 100644
>> --- a/modules/database/src/ioc/db/db_field_log.h
>> +++ b/modules/database/src/ioc/db/db_field_log.h
>> @@ -81,6 +92,8 @@ struct dbfl_val {
>>   * db_delete_field_log().  Any code which changes a dbfl_type_ref
>>   * field log to another type, or to reference different data,
>>   * must explicitly call the dtor function.
>> + * If the dtor is NULL, then this means the array data is still owned
>> + * by a record.
> 
> Would it make sense to provide a macro that evaluates that condition? The code I saw earlier that I assume was checking this looked somewhat scary for after a clean-up.

I guess you mean e.g. in dbAccess.c:

+    if ((!pfl || (pfl->type==dbfl_type_ref && !pfl->u.r.dtor)) &&
+        paddr->pfldDes->special == SPC_DBADDR &&
+        (prset = dbGetRset(paddr)) &&
+        prset->get_array_info) {
+        status = prset->get_array_info(paddr, &no_elements, &offset);
+    } else {
         offset = 0;
     }

Yes, it probably isn't too obvious to the reader what goes on here. In a
later patch, I have used a local boolean variable a la

+    int pfl_has_copy = pfl && (pfl->type != dbfl_type_ref ||
pfl->u.r.dtor);

but I guess offering (and using) a macro here is the right thing. Will fix.

>> +<<<<<<< modules/database/src/std/filters/arr.c
> 
> I think the conflict markers shown here are really virtual, I believe they will disappear if/when we merge Dirk's branch that is marked as a prerequisite of this one.

The conflict is with a change you made on 7.0 since Dirk has created his
feature branch:

commit 25bb966cbc4be1f2a47116a8b9ec940bc2270b12
Author: Andrew Johnson <anj at aps.anl.gov>
Date:   Sat Mar 14 16:19:26 2020 -0500

    Use the dbChannel*() accessor macros in the array filter code
    instead of exposing the dbChannel innards unnecessarily.

I wasn't sure how to set up my branch so that it would include Dirk's
changes /and/ all the latest changes from 7.0. (One possibility: fork
off my branch at the head of 7.0, then first merge Dirk's branch before
adding my own commits.)

If you have a recommendation to share, please do so.

-- 
https://code.launchpad.net/~bfrk/epics-base/+git/epics-base/+merge/381464
Your team EPICS Core Developers is requested to review the proposed merge of ~bfrk/epics-base:remove-dbfl_type_rec into epics-base:7.0.

Experimental Physics and Industrial Control System

Experimental Physics and
Industrial Control System