Questionable data type in devexplore.cpp template readArray and template read

[dirk-zimoch]

    template<typename VAL>
    unsigned readArray(VAL *val, unsigned count) const
    {
        epicsUInt32 addr = 0,
                    end  = barsize-offset;
        unsigned i;
        for(i=0; i<count && addr<end; i++, addr+=step)
        {
            epicsUInt32 OV = read<VAL>(addr);
            *val++ = castval<VAL,epicsUInt32>::op(OV);
        }
        return i;
    }

When instatiated with epicsFloat32 in line 155, strange conversions are going on (and some compilers warn) read<VAL> returns epicsFloat32 which is then cast to epicsUInt32, then back to epicsFloat32.

Also, in template read VAL is copied but not cast to a epicsUInt32, which generates a warning for VAL=epicsFloat32.

I find all this casting between epicsUInt32 and epicsFloat32 across 3 templates quite questionable.

As readraw reads epicsUInt32 and then casts to VAL, there is no reason for read to call readraw<VAL>. It should call readraw<epicsUInt32>. For the same reason, readArray should call read<epicsUInt32> instead of read<VAL>.

[mdavidsaver]

This does look strange. It's been 4 years since I added this, and I must admit that I'm having trouble remembering the details. The situation I faced at the time involved a custom device with real ieee floating point registers.

[dirk-zimoch]

For a device with real floating point registers the code won't work. The registers are always read as integers and stored in epicsUInt32 OV. Casting them to float is of course not the same as reading them as float. That would require a readraw (or read) version which uses for example a union to reinterpret the value as float.

[mdavidsaver]

There is a union hiding in the castval<> template. So it does work, at least on Linux. I'm working on adding a test case to demonstrate this.

https://github.com/epics-modules/devlib2/blob/5d28689a94348d58d215df1e115950cc2ae71782/exploreApp/src/devexplore.cpp#L75-L83

Of course, I must acknowledge that there are unnecessary casts happening, and that the appearance of correctness may well depend on how GCC optimizes these casts. There is room for improvement.

[dirk-zimoch]

Oops. I missed that. Sorry.

[mdavidsaver]

I may have addressed this with 614e0a8dfd1c061135d9c64627b58ccfdbf80c00. Do you still see this warning? If so, which warning is it? It would help for me to know what to look for in eg. the appveyor output.

[dirk-zimoch]

I used to get the warnings from the Microsoft compiler (in both, 32 bit and 64 build). It is very sensitive to potential data loss due to implicit casting, e.g. 32 bit integer to 32 bit float.

/opt/wine-msvc-2017/bin/x86/cl -EHsc -GR            -DUSE_TYPED_RSET -I../../../common     -nologo -FC -D__STDC__=0 -D_CRT_SECURE_NO_DEPRECATE -D_CRT_NONSTDC_NO_DEPRECATE    -Ox -GL -Oy-   -W3 -w44355 -w44344 -w44251     -D_SILENCE_TR1_NAMESPACE_DEPRECATION_WARNING      -MD -DEPICS_BUILD_DLL -DEPICS_CALL_DLL -TP  -I. -I../O.Common -I. -I. -I.. -I../../../include/compiler/msvc -I../../../include/os/WIN32 -I../../../include -I/usr/local/epics/base-7.0.4.1/include/compiler/msvc -I/usr/local/epics/base-7.0.4.1/include/os/WIN32 -I/usr/local/epics/base-7.0.4.1/include        -c ../devexplore.cpp
devexplore.cpp
z:\afs\psi.ch\group\8211\dirk\git\devlib2\exploreapp\src\devexplore.cpp(154): warning C4244: 'initializing': conversion from 'VAL' to 'epicsUInt32', possible loss of data
        with
        [
            VAL=epicsFloat32
        ]
z:\afs\psi.ch\group\8211\dirk\git\devlib2\exploreapp\src\devexplore.cpp(475): note: see reference to function template instantiation 'unsigned int `anonymous-namespace'::priv::readArray<epicsFloat32>(VAL *,unsigned int) const' being compiled
        with
        [
            VAL=epicsFloat32
        ]
z:\afs\psi.ch\group\8211\dirk\git\devlib2\exploreapp\src\devexplore.cpp(143): warning C4244: 'return': conversion from 'epicsUInt32' to 'VAL', possible loss of data
        with
        [
            VAL=epicsFloat32
        ]
z:\afs\psi.ch\group\8211\dirk\git\devlib2\exploreapp\src\devexplore.cpp(154): note: see reference to function template instantiation 'VAL `anonymous-namespace'::priv::read<VAL>(epicsUInt32) const' being compiled
        with
        [
            VAL=epicsFloat32
        ]
z:\afs\psi.ch\group\8211\dirk\git\devlib2\exploreapp\src\devexplore.cpp(475): note: see reference to function template instantiation 'unsigned int `anonymous-namespace'::priv::readArray<epicsFloat32>(VAL *,unsigned int) const' being compiled
        with
        [
            VAL=epicsFloat32
        ]

I had as well seen this warning from g++ 3.4.3

../devexplore.cpp: In member function `epicsUInt32 <unnamed>::priv::readraw(epicsUInt32) const':
../devexplore.cpp:118: warning: converting of negative value `-0x00000000000000001' to `epicsUInt32'
../devexplore.cpp: In member function `unsigned int <unnamed>::priv::readArray(VAL*, unsigned int) const [with VAL = epicsFloat32]':
../devexplore.cpp:475:   instantiated from here
../devexplore.cpp:154: warning: converting to `epicsUInt32' from `epicsFloat32'

Both are gone now