Template parameter with variadics and shared_ptr

Pages: 12
There is a problem with the polymorphic type code above. It seems if I create a third wrapper object :

 
wrapper<Sensor_packet> wcliff=make_object<Sensor_packet_v>("CLIFF", 1);


And I create behavior2 :

1
2
Behavior b2( &Robot::check_sensor< wwheel, wbumper, wcliff >, robot );
 b2.exec();


The result is :
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
Initial buffer for sensor_packet_b WHEEL 00000001
Initial buffer for sensor_packet_v BUMPER 00000010
Initial buffer for sensor_packet_v CLIFF 00000010
Changing WHEEL to 0 
Changing CLIFF to 0   //this should be BUMPER,  check_sensor stepped past BUMPER to get CLIFF
Checking results
WHEEL shows 0
CLIFF shows 0  //this should be BUMPER
It took me 7 clicks (0.000007 seconds).
Changing WHEEL to 0 
Changing CLIFF to 0   //this should be BUMPER
Changing CLIFF to 0  //this is correct
Checking results
WHEEL shows 0
CLIFF shows 0  //this should be BUMPER
CLIFF shows 0
signed int is lock free : 1
unsigned char is lock free: 1


It seems the wrapper objects are created properly but they are not received at check_sensor correctly. The union class method does not have this issues. I suspect the reason is because for the union class method, the objects sent to check_sensor are of uniform size. So it seems collapsing objects to a common base pointer cannot work in this situation.

Only solution left is creating a union sensor_packet class with std::atomic type union or union members.


Chris
> I gathered that the reason for no copy constructor or assignment operators with std::atomic types
> is because some may require a mutex (not lock free) for atomicity.
> So the general case must not have copy constructors or assignment operators.

The copy operations are deleted because there is no mechanism to atomically copy the value fropm one atomic object to another. With atomic objects a and b, both a.load() and b.store() are atomic in isolation,
but b.store( a.load() ) taken together is not a single atomic operation.

1
2
buffer.store( pOther.buffer.load( std::memory_order::memory_order_seq_cst ), 
              std::memory_order::memory_order_seq_cst ); 

is the same as buffer = pOther.buffer.load();

In the posted snippet, we can replace the dubious
1
2
3
4
5
6
7
8
9
template <class U>
U& make_object(std::string id, int x){
    static U object;
    object=U(id,x);
    return object;
}

wrapper<Sensor_packet> wwheel=make_object<Sensor_packet_b>("WHEEL", 0);
wrapper<Sensor_packet> wbumper=make_object<Sensor_packet_v>("BUMPER", 1);

with
1
2
3
4
5
6
7
namespace  {
    Sensor_packet_b wheel_("WHEEL", 0);
    Sensor_packet_v bumper_("BUMPER", 1);
}

wrapper<Sensor_packet> wwheel(wheel_) ;
wrapper<Sensor_packet> wbumper(bumper_);

and we won't need the copy operations (we can let them be be implicitly deleted).


> There is a problem with the polymorphic type code above.
> It seems if I create a third wrapper object ...

The problem is due to make_object() - in this instance, make_object<Sensor_packet_v>() always returns a reference to the same object (after overwriting it).

Which is why I called it 'dubious' in the previous post.
You're right. So is there a way to create a static or global object from within a function that is allocated on the stack, that will persist after the function returns ?

With the changes you suggested, I think everything is working fine now as far as I can tell with the polymorphic version. Since there are no copy/assignment operations necessary, then sensor_packet.buffer reads and writes should complete sequentially. The race condition is resolved.

With the union class method, is it sufficient to just :

1
2
3
4
5
     union
    {
       std::atomic<bool> bit{true} ; // type == bool_type
       std::atomic<int> int_value ; // type == int_type
    };


It seems to run fine after bit is switched to direct initialization.

Thanks,
Chris
> With the union class method, is it sufficient to just :
1
2
3
4
5
> union
> {
>      std::atomic<bool> bit{true} ; // type == bool_type
>      std::atomic<int> int_value ; // type == int_type
> };


For maintainability, it would be a good idea to add this to the Sensor_packet class:
1
2
3
static_assert( std::is_trivially_destructible< std::atomic<bool> >::value &&
               std::is_trivially_destructible< std::atomic<int> >::value,
               "we need to provide a user-defined destructor for Sensor_packet" ) ;


Note that while this makes the two members atomic, operations which modify the sensor packet as a whole (for instance, if both the packet type as well as the value in the union are modified) would not be atomic.

In in the code below, the set() functions would not be atomic, and the stream insertion operator could have race conditions:
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
#include <string>
#include <atomic>
#include <type_traits>
#include <utility>
#include <iostream>
#include <iomanip>

struct Sensor_packet
{
    enum packet_type { bool_type, int_type };

    std::string id ;

    packet_type type = bool_type ;

    static_assert( std::is_trivially_destructible< std::atomic<bool> >::value &&
                   std::is_trivially_destructible< std::atomic<int> >::value,
                   "we need to provide a user-defined destructor for Sensor_packet" ) ;
    union
    {
       std::atomic<bool> bit { true } ; // type == bool_type
       std::atomic<int> int_value ; // type == int_type
    };

    Sensor_packet( std::string id ) : id( std::move(id) ) {}

    Sensor_packet( std::string id, std::pair< unsigned char, std::size_t > bit_and_offset )
        : id( std::move(id) ), type(bool_type),
          bit( nth_bit( bit_and_offset.first, bit_and_offset.second ) ) {}

    Sensor_packet( std::string id, int value )
        : id( std::move(id) ), type(int_type), int_value(value) {}

    int get() const { return bool_type ? bit : int_value ; }

    void set( std::pair< unsigned char, std::size_t > bit_and_offset ) {

        type = bool_type ;
        bit = nth_bit( bit_and_offset.first, bit_and_offset.second ) ;
    }

    void set( int value ) {

        type = int_type ;
        int_value = value ;
    }

    static bool nth_bit( unsigned char bits, std::size_t bit_offset )
    { return bit_offset < 8U ? bits & ( 1U << bit_offset ) : false ; }

    friend std::ostream& operator<< ( std::ostream& stm, const Sensor_packet& pkt )
    {
        stm << "{ " << std::quoted(pkt.id) << ", " << std::boolalpha ;
        if( pkt.type == pkt.bool_type ) return stm << "bool:" << pkt.bit << " )" ;
        else return stm << "value:" << pkt.int_value << " }" ;
    }
};



If we need to guard everything in Sensor_packet (including the string member), more coarse-grained locking (with poorer performance) would be required. For instance:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
#include <mutex>
#include <string>
#include <utility>
#include <iostream>
#include <iomanip>

struct Sensor_packet
{
    Sensor_packet( std::string id ) : id( std::move(id) ) {}

    Sensor_packet( std::string id, std::pair< unsigned char, std::size_t > bit_and_offset )
        : id( std::move(id) ), type(bool_type),
          bit( nth_bit( bit_and_offset.first, bit_and_offset.second ) ) {}

    Sensor_packet( std::string id, int value )
        : id( std::move(id) ), type(int_type), int_value(value) {}

    const std::string& name() const { return id ; }
    void name( const std::string& new_id ) { lock_type lock(mutex) ; id = new_id ; }

    int get() const { lock_type lock(mutex) ; return bool_type ? bit : int_value ; }

    void set( std::pair< unsigned char, std::size_t > bit_and_offset ) {

        lock_type lock(mutex) ;
        type = bool_type ;
        bit = nth_bit( bit_and_offset.first, bit_and_offset.second ) ;
    }

    void set( int value ) {

        lock_type lock(mutex) ;
        type = int_type ;
        int_value = value ;
    }

    static bool nth_bit( unsigned char bits, std::size_t bit_offset )
    { return bit_offset < 8U ? bits & ( 1U << bit_offset ) : false ; }

    friend std::ostream& operator<< ( std::ostream& stm, const Sensor_packet& pkt )
    {
        Sensor_packet::lock_type lock( pkt.mutex ) ;
        stm << "{ " << std::quoted(pkt.id) << ", " << std::boolalpha ;
        if( pkt.type == pkt.bool_type ) return stm << "bool:" << pkt.bit << " )" ;
        else return stm << "value:" << pkt.int_value << " }" ;
    }

    private:
        mutable std::mutex mutex ;
        using lock_type = std::lock_guard< decltype(mutex) > ;

        enum packet_type { bool_type, int_type };

        std::string id ;

        packet_type type = bool_type ;

        union
        {
           bool bit { true } ; // type == bool_type
           int int_value ; // type == int_type
        };
};



If the string data member is never going to be modified, but both the type and the members of the union may be modified: make the id a const object and wrap both the type member and the union into a single
std::atomic<user-defined-type> object. This would be much more efficient than using a mutex. For instance:

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
#ifdef _MSC_FULL_VER
    #define _ENABLE_ATOMIC_ALIGNMENT_FIX 1 // required only for for microsoft C++
    // see: https://blogs.msdn.microsoft.com/vcblog/2016/04/14/stl-fixes-in-vs-2015-update-2/
#endif

#include <atomic>
#include <string>
#include <utility>
#include <iostream>
#include <iomanip>
#include <type_traits>

struct Sensor_packet
{
    enum packet_type { bool_type, int_type };

    const std::string id ;

    Sensor_packet( std::string id ) : id( std::move(id) ) {}

    Sensor_packet( std::string id, std::pair< unsigned char, std::size_t > bit_and_offset )
        : id( std::move(id) ), data(bit_and_offset) {}

    Sensor_packet( std::string id, int value ) : id( std::move(id) ), data(value) {}

    int get() const { return data.load().get() ; }

    void set( std::pair< unsigned char, std::size_t > bit_and_offset )
    { data = data_( bit_and_offset ) ; }

    void set( int value ) { data = data_( value ) ; }

    bool is_lock_free() const { return data.is_lock_free() ; }

    friend std::ostream& operator<< ( std::ostream& stm, const Sensor_packet& pkt )
    {
        stm << "{ " << std::quoted(pkt.id) << ", " << std::boolalpha ;
        const Sensor_packet::data_ data = pkt.data ;
        if( data.type == pkt.bool_type ) return stm << "bool:" << bool(pkt.get()) << " }" ;
        else return stm << "value:" << pkt.get() << " }" ;
    }

    private:

        struct data_
        {

            data_() noexcept = default ; // note: noexcept is actually required only for the GNU compiler
                                         // without it, the GNU offering deduces the exception specification wrongly

            data_( std::pair< unsigned char, std::size_t > bit_and_offset ) noexcept
                : bit( nth_bit( bit_and_offset.first, bit_and_offset.second ) ) {}

            data_( int value ) noexcept : type( int_type ), int_value( value ) {}

            packet_type type = bool_type ;
            union { bool bit { true } ; int int_value ; };

            int get() const { return bool_type ? bit : int_value ; }

            static bool nth_bit( unsigned char bits, std::size_t bit_offset )
            { return bit_offset < 8U ? bits & ( 1U << bit_offset ) : false ; }
        };

        static_assert( std::is_trivially_destructible< std::atomic<data_> >::value,
                       "we need to provide a user-defined destructor for Sensor_packet::data_" ) ;

        std::atomic<data_> data ;
};

http://coliru.stacked-crooked.com/a/10ff91e90cc98805
@JLBorges

the set() functions would not be atomic, and the stream insertion operator could have race conditions:


1
2
3
4
5
6
7
8
9
10
11
void set( std::pair< unsigned char, std::size_t > bit_and_offset ) {

        type = bool_type ;
        bit = nth_bit( bit_and_offset.first, bit_and_offset.second ) ;
    }

void set( int value ) {

        type = int_type ;
        int_value = value ;
    }


Q. Is it because the type is not atomic that the set functions are not atomic?

If we need to guard everything in Sensor_packet (including the string member), more coarse-grained locking (with poorer performance) would be required. For instance:


1
2
3
4
5
6
7
8
9
10
11
12
13
 void set( std::pair< unsigned char, std::size_t > bit_and_offset ) {

        lock_type lock(mutex) ;
        type = bool_type ;
        bit = nth_bit( bit_and_offset.first, bit_and_offset.second ) ;
    }

    void set( int value ) {

        lock_type lock(mutex) ;
        type = int_type ;
        int_value = value ;
    }


Q. Is mutex is slower than an atomic type or is it slower because both the type and the bit/int_value are being guarded?


If the string data member is never going to be modified, but both the type and the members of the union may be modified: make the id a const object and wrap both the type member and the union into a single
std::atomic<user-defined-type> object. This would be much more efficient than using a mutex.


Q. I did not know custom atomic types can be created. Can you suggest an implementation to protect a serial port? I might switch to that solution if it is faster than a mutex. Right now I am using a lock_guard recursive mutex for writes to the devicedescriptor which is a type int. Is it as simple as making devicedescriptor atomic<int> and dropping the mutex? Does guarding the int assure that byte reads and writes are atomic?

1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
int Biscuit::biscConnect(char const *device) {
    if(device == NULL) return BISC_ERR;

    struct termios tty;

    // Try to open the device
    if((deviceDescriptor = open(device, O_RDWR | O_NOCTTY | O_NONBLOCK)) == -1) {
        return BISC_ERR;
    }

    tcgetattr(deviceDescriptor, &tty);
    tty.c_iflag     = IGNBRK | IGNPAR;
    tty.c_lflag     = 0;
    tty.c_oflag     = 0;
    tty.c_cflag     = CREAD | CS8 | CLOCAL;
    
    cfsetispeed(&tty, BISC_BAUD_RATE);
    cfsetospeed(&tty, BISC_BAUD_RATE);
    tcsetattr(deviceDescriptor, TCSANOW, &tty);
    
    return deviceDescriptor;
}


int Biscuit::biscReceiveByte(unsigned char  &byte) {
    std::lock_guard<std::recursive_mutex> lock(s_mutex);
    usleep(10000);
    return (read(deviceDescriptor, &byte, 1) == 1 ? BISC_SUCCESS : BISC_ERR);
}

int Biscuit::biscSendByte(uint8_t byte) {
    std::lock_guard<std::recursive_mutex> lock(s_mutex);
    usleep(10000);
    return (write(deviceDescriptor, &byte, 1) == 1 ? BISC_SUCCESS : BISC_ERR);
}



Q. What is the reason for the static_assert ? Won't the default destructor suffice?

I have utilized concepts I learned above into my project. Check_sensor seems to be working properly. I am doing a massive overhaul of the code. I was able to reduce polymorphic heirarchy to two levels from three by using as non type template arguments the function pointers I normally pass through the constructor and then using a typedefs to refer to the instantiated classes. I converted the Robot member of Behavior to a static member so I wont have to pass a Robot instance to the Behavior constructor each time I create a Behavior object. This necessitated a switch to a Robot pointer from Robot reference static class member. Now the only thing I pass to the constructor is the std::string. Too bad there is no way in c++ to obtain a variable's name.
I tried to take a stab at converting Behavior objects from shared_ptr to references but I cannot create vectors of references without reference_wrapper. I did not know that was necessary because the shared_ptr use kept that issue from surfacing. I decided to stick with shared_ptr. I will be looking for vectors and attempting to convert them to arrays or if that is not possible, reserve a reasonable vector size.

Thanks,
Chris
> Q. Is it because the type is not atomic that the set functions are not atomic?

No. Even if individual operations on two objects are atomic, a compound operation involving both need not be atomic.

With
1
2
3
4
5
void set( int value ) {

        type = int_type ; // type is an atomic object
        int_value = value ; // int_value is also an atomic object
    }


For instance, we may get this sequence of operations:
1. Thread a atomically stores int_type in type
2. Thread b atomically stores bool_type in type
3. Thread b atomically stores true in bit
4. Thread a atomically stores value in int_value


> Is mutex is slower than an atomic type or is it slower because both the type and the bit/int_value are being guarded?

Typically, an atomic object which is lock free is faster than an object guarded by a mutex.
http://en.cppreference.com/w/cpp/atomic/atomic/is_lock_free

More information: http://www.cplusplus.com/forum/general/194170/#msg933840

Ckeck if Sensor_packet::data_ is lock free:
http://coliru.stacked-crooked.com/a/e6c67c62223980cd
http://rextester.com/MBGL22613


> Can you suggest an implementation to protect a serial port?

The requirement to create the atomic type std::atomic<serial_port> is that the type serial_port is TriviallyCopyable

Note: since copying serial ports would not be semantically correct, you would have to guard it indirectly through a TriviallyCopyable wrapper: for instance through,
std::atomic< std::reference_wrapper<serial_port> >


> What is the reason for the static_assert ? Won't the default destructor suffice?

If a union contains a non-static data member with a non-trivial special member function (default constructor, copy/move constructor, copy/move assignment, or destructor), that function is deleted by default in the union and needs to be defined explicitly by the programmer.
http://en.cppreference.com/w/cpp/language/union

I have gotten pretty busy with work and other things lately. I'm trying to understand the static assert. I think that if the union comprised of just an int and a bool, then these types have trivial constructors therefore, no need to create a destructor. If the type is an atomic version of int or bool or any other custom type, then the object created using an atomic template class instantiation of that type results in a class that has special (non trivial) constructors.


Atomic types:
These specializations have standard layout, trivial default constructors, and trivial destructors"


std::atomic is neither copyable nor movable.


Unions cannot contain a non-static data member with a non-trivial special member function (copy constructor, copy-assignment operator, or destructor).


If a union contains a non-static data member with a non-trivial special member function (default constructor, copy/move constructor, copy/move assignment, or destructor), that function is deleted by default in the union and needs to be defined explicitly by the programmer.




Due to the non trivial copy constructor and move constructor which are deleted in atomic types, the destructor for sensor_packet needs to be defined explicitly. Is this correct?

How can this be defined? There are no pointers to delete. Does it suffice to just write an empty destructor?

Thanks,
Chris



> Due to the non trivial copy constructor and move constructor which are deleted in atomic types,
> the destructor for sensor_packet needs to be defined explicitly. Is this correct?

No.

Because the copy constructor and move constructor which are deleted in atomic types, the copy constructor and move constructor are implicitly deleted in sensor_packet.

The members of the anonymous union union { bool bit { true } ; int int_value ; }; are trivially destructible, and consequently sensor_packet::data_ is also implicitly declared and trivial. (This would not have been the case if a non-static member of the union in sensor_packet::data_ was not trivially destructible.)

We would have to provide a user-defined destructor for sensor_packet::data_ only if the static assertion fails.
Topic archived. No new replies allowed.
Pages: 12