Using Explicit

In looking at how to put together my own library as instructed in

I noticed an error in the constructor

class Morse
{
  public:
    Morse(int pin);
    void dot();
    void dash();
  private:
    int _pin;
};

The code could be made much better by making the constructor explicit. As it stands, the constructor could be used by the compiler to cast an int into a Morse. For example, the compiler would be allowed to coerce the right-hand side of the assignment, which is an int, into a Morse on the left-hand side:

Morse m = 13;

However, it does not make any semantic sense to assign an int to a Morse-code object. It's a potential source of coding errors especially if the programmer meant to write

int m = 13;

The constructor ought to be changed to read

    explicit Morse(int pin);

With the constructor now made explicit, it is not possible for the compiler to cast an int into a Morse. It is not possible to write Morse m = 13. The programmer must write it in a conventional fashion

Morse m(13);

which is much clearer. The semantics are now clear: the Morse-code object, m, is constructed to use pin 13.

Don't see it as a problem myself.

class morse_t
{
    const uint8_t   _pin;

public:
    morse_t(uint8_t const pin)
        // use initialization list, 'const's must be init'd first
        : _pin(pin)
    {}

    void begin()
    {
        pinMode(_pin, OUTPUT);
    }

    void dot();
    void dash();
};

morse_t     morse0(13);     // valid code
morse_t     morse1 = 13;    // valid code

void morse_t::dot()
{   }

void morse_t::dash()
{   }


void loop()
{    }

void setup()
{
    morse0.begin();
    morse1.begin();
}

If you aren't going to allow implicit use of constructors, be nice and add in an assignment operator. However as the pin is const the best option is to allow C++ to do its job. A well defined class enables the compiler to select the most efficient option and remove elements it doesn't need ( m=x could implicitly become m(x) ), but may select differently if you also used an assignment operator, which could reduce code duplication.

If you are worried about inputs, validate them in the constructor, which you still can't do due to being marked const.

If you insist on the pin being const, then maybe a template would be worth investigation.
The below is called a non-type template ( takes a value rather than a type in the template specification. )

template< unsigned char _P > class Morse{
  public:
    Morse( void );

    void dot(){ 
        digitalWrite( _P, HIGH ); //Use _P directly, no need for instance variable.
        digitalWrite( _P, LOW );
    }

    void dash();
};

Morse< 13 > m; //No specific or copy constructor.

**EDIT:**If you intend on using more than one instance of the library, make the pin variable a 'variable' rather than const, then you can re-use an actual object, which is closer to the paradigm of code re-use.

pYro_65:
make the pin variable a 'variable' rather than const, then you can re-use an actual object

It seems pretty strange to me to expect to detach and reattach objects to different pins. If it was sensible in a given application there's nothing wrong with supporting it, but I wouldn't consider this as normal.

pYro_65:
which is closer to the paradigm of code re-use.

Not in any sense that I would use the term code reuse.

PeterH:
It seems pretty strange to me to expect to detach and reattach objects to different pins. If it was sensible in a given application there's nothing wrong with supporting it, but I wouldn't consider this as normal.

It is in no way abnormal. Reusing an objects lifetime can almost always be of benefit.
Consider a few examples:

  • An object created on the heap, which has a fixed lifetime.
  • A dynamically allocated object ( heap memory ) that has its lifetime re-used will prevent fragmentation; which could occur when indiscriminately throwing away objects, only to recreate them with different parameters.
  • A large object that is too big to justify multiple instances, or is just too big to fit in ram multiple times.
  • An object that is large or convoluted may take a long time to initialise/destroy, in time critical situations this can bring execution to a crawl. When comparing a write of a few primitives to creation/destruction of an object, there can be very noticeable differences.

If you want to protect a variable from being written, declare it as protected or private, then use member functions to access it. Inline functions simply returning a reference to a member are no less efficient than accessing its public counterpart directly.
You can justify const when using structs, or when applying it to things that are truly const, for example: your SREG register, or extended memory start will never change its location, which would justify a constant pointer.

PeterH:

pYro_65:
which is closer to the paradigm of code re-use.

Not in any sense that I would use the term code reuse.

Once you use const in an object, especially on a value supplied by the client, you are making part of its functionality 'single-use', which instantly demotes it from being well-defined reusable code, as in you have to recreate it to reuse it.

pYro_65:
Once you use const in an object, especially on a value supplied by the client, you are making part of its functionality 'single-use', which instantly demotes it from being well-defined reusable code, as in you have to recreate it to reuse it.

You seem to be describing object reuse. Which is all well and good in the appropriate circumstances, but I don't see that it has anything to do with code reuse.

In C++ object reuse applies to inheritance, not implementation.

The basis of reusing an objects lifetime is inherently code reuse. You can only reuse an object when the implementation allows it, which is its definition or composition. Code reuse in C++ is achieved via composition, therefore a class that allows itself to be reused is reusable code.

To close the loop on this discussion, I have changed the implementation of Morse.h and Morse.cpp as shown below.

I do wish to associate myself with PeterH's comments. I agree with PeterH.

One issue that has not be raised in this discussion is thread safety. True, on an Arduino, we do not face the same kinds of threading and synchronization issues as we do on modern mult-core processors. Still, it's worthwhile to design classes that are thread safe. The Morse class can only be thread safe if _pin is made const and there is nothing in the class's interface to permit changes. The class is more reliable in a multi-threading context if the object is immutable. Immutable objects are quite common in C++.

It is part of the contract of the dot() and dash() functions that _pin's value will not change while the functions block on delay(). If there were a way to permit _pin's value to be changed by another thread while the thread which called dot() or dash() is blocked, the old pin would become orphaned and left in a HIGH state likely causing an error in the hardware. Synchronization issues are typically very hard-to-find, and the class's designer has an obligation to prevent such errors from happening.

Furthermore, there is no legitimate reason to have more than one Morse object associated with any pin. So, I have intentionally made its copy constructor and operator= private. The class is non-copyable. Non-copyable classes are quite legitimate in C++. So, this kind of statement is prohibited

Morse mAlias = m13;

where m13 is an existing Morse object. Having mAlias serves no purpose, and it is a potential source for bugs.

I have left the explicit on the Morse constructor, because there is no legitimate reason to permit the compiler to cast ints into Morse objects. I believe that if a client can call a constructor with a single parameter and there is no reason for the compiler to be allowed to make a cast, then the cast should be prohibited through the use of explicit.

I also made a fix to the constructor in Morse.cpp such that _pin is set in an initialization list -- not in the body of the constructor which is not best practice.

#ifndef Morse_h
#define Morse_h

#include "Arduino.h"

class Morse
{
  public:
    explicit Morse(int pin);
    void dot();
    void dash();
  private:
    Morse(const Morse&);
    Morse& operator=(const Morse&);	
    const int _pin;
};

#endif
#include "Arduino.h"
#include "Morse.h"

Morse::Morse(int pin) : _pin(pin)
{
  pinMode(_pin, OUTPUT);
}

void Morse::dot()
{
  digitalWrite(_pin, HIGH);
  delay(250);
  digitalWrite(_pin, LOW);
  delay(250);  
}

void Morse::dash()
{
  digitalWrite(_pin, HIGH);
  delay(1000);
  digitalWrite(_pin, LOW);
  delay(250);
}

You might have a bit of a point there. Calling it an error is perhaps going to far (the compiler didn't complain) but something to be avoided, perhaps. This rather contrived test demonstrates:

class Morse
{
  public:
    Morse(int pin) : pin_ (pin), dotCount_ (0) { }
    int dot () { return ++dotCount_; }
  private:
    int pin_;
    int dotCount_;
};

Morse m = 13;

void setup () 
  {
  Serial.begin (115200);
  m.dot ();
  Serial.println (m.dot ());
  m = 14;
  Serial.println (m.dot ());
  }
void loop () { }

I'm not a big fan of leading underscores BTW, they are close to being compiler reserved variables. Try the trailing underscore.

This example prints:

2
1

The "2" I expect, but the "1" shows that "m" was re-constructed by the line "m = 14;" which perhaps was not intended. In fact that line would be an odd thing to do, one suggests.


This variation shows some interesting things:

class Morse
{
  public:
    Morse(int pin) : pin_ (pin), dotCount_ (0) { Serial.println  ("constructor"); }
    ~Morse () { Serial.println  ("destructor"); }
    int dot () { return ++dotCount_; }
  private:
    int pin_;
    int dotCount_;
};


void setup () 
  {
  Serial.begin (115200);

  Morse m = 13;
  m.dot ();
  Serial.println (m.dot ());
  m = 14;
  Serial.println (m.dot ());
  }

void loop () { }

Output:

constructor
2
constructor
destructor
1
destructor

Clearly m is being created twice. And the second time it is created before the first one was destroyed. One wonders what that would do to class variables you might be keen to have valid, like pointers.

You may want to re-think your approach. It is a little flawed.

The Morse class can only be thread safe if _pin is made const and there is nothing in the class's interface to permit changes.

const does not guarantee this! Think of the few scenarios:

  • Thread A owns the object thread B uses, or
  • Global memory has the object, both threads share, or
  • The opposite of the first point.

const prevents nothing from destroying the object while another thread is in the middle of using it. If the object is owned by a thread, what is stopping the thread from dying prematurely. If you can destroy the object without a thread knowing, then you can recreate it with different parameters, breaking your contract to the thread using it.

To protect data in a threaded environment you must use the blocking methods. Either through wait methods or mutual exclusion like semaphores or a mutex.

Furthermore, there is no legitimate reason to have more than one Morse object associated with any pin. So, I have intentionally made its copy constructor and operator= private. The class is non-copyable. Non-copyable classes are quite legitimate in C++. So, this kind of statement is prohibited

Maybe not, but multiple objects set to different pins is a valid possibility.

Clearly m is being created twice. And the second time it is created before the first one was destroyed. One wonders what that would do to class variables you might be keen to have valid, like pointers.

@Nick, I assume you already get this, but for clarity.
That outcome is due to using code that hasn't been written. To assign to an already initialized object you must have an assignment operator, which there is none.

Morse m = 13;   //Initialisation
m = 14;  //Assignment

pYro_65:
@Nick, I assume you already get this, but for clarity.
That outcome is due to using code that hasn't been written. To assign to an already initialized object you must have an assignment operator, which there is none.

What do you mean "hasn't been written"? My example stands on its own. It is written.

Hi pyro_65, Hi Nick.

Obviously, we would need to use the boost libraries to finish off the thread safety. That's not part of Arduino from what I can see.

For example, we do need to place a mutex on dot() and dash(). We would not want two or more threads simultaneously entering those functions. Otherwise, the pin's in HIGH and LOW might not be set for the required time. One thread must certainly wait and block before the next thread executes it.

I took it for granted that that was understood. Thank you for giving me the opportunity to clarify. Obviously there is more work needed here with boost.

Still, it is correct practice to make the _pin const to ensure that another thread could not orphan the pin and also this practice makes the class designer's intentions clear. I believe that an immutable and non-copyable object is still the best design choice for the class. It also avoids possible errors with the client using the class. Allowing the client to change the pin on the same object is just another source of potential error. It is far far better to use an immutable object, helping the client to avoid errors.

Maybe not, but multiple objects set to different pins is a valid possibility.

There is absolutely no reason to have multiple objects set to the same pin. It's a potential source of confusion and errors. Certainly, multiple objects set to different pins is very much a possibility; it is the practice I would recommend.

I believe that the best approach is a non-copyable object (which is also more easily achieved through the boost libraries) and immutable.

Myself, I have never had any difficulties with leading underscores Nick. I suppose it's a matter of personal taste. However, it does bring to mind that to improve my code, I really should place the class in a namespace to avoid possible name collisions with other libraries the client might include.

Thank you pyro65 and Nick for sharing. :slight_smile:

A more elaborate test shows this:

class Morse
{
  public:
    Morse(int pin) : pin_ (pin), dotCount_ (0) 
      { 
      Serial.print  ("constructor, this = "); 
      Serial.println ((unsigned long) this, HEX);
      }
    ~Morse () 
      { 
      Serial.print  ("destructor, this = "); 
      Serial.println ((unsigned long) this, HEX);
      }
    int dot () 
      { 
      Serial.print  ("in 'dot', this = "); 
      Serial.println ((unsigned long) this, HEX);
      return ++dotCount_; 
      }
  private:
    int pin_;
    int dotCount_;
};


void setup () 
  {
  Serial.begin (115200);

  Morse m = 13;
  m.dot ();
  Serial.println (m.dot ());
  m = 14;
  Serial.println (m.dot ());
  }

void loop () { }

Output:

constructor, this = 8EA
in 'dot', this = 8EA
in 'dot', this = 8EA
2
constructor, this = 8EE
destructor, this = 8EE
in 'dot', this = 8EA
1
destructor, this = 8EA

It appears that the "real" m is at 8EA. When this line is executed:

  m = 14;

... a temporary copy of the class instance is created at 8EE, then assigned to 8EA (presumably by a bit-wise copy). The temporary copy is then destroyed. However because of the assignment the variable dotCount_ is now back to zero.

This may quite well be not what you expect.

I did quite a few tests of this sort of thing here:

Your example does not include the assignment operator, therefore what are you even expecting?

Obviously, we would need to use the boost libraries to finish off the thread safety. That's not part of Arduino from what I can see.

The boost library is a helper tool, its not required for thread safety. If you use an RTOS or some other system supporting threading, they will provide their own method of mutual exclusion.

Allowing the client to change the pin on the same object is just another source of potential error.

Not at all, bad design is your problem there. Assignment operators are there to fix the error nick encountered, and to safely reuse objects.

Classes provide encapsulation for a reason, the private section is there for your purpose, so use it. There still has not been a remarkably valid reason to justify a const pin, all it is doing is hindering your own codes potential. Other programmers there to modify your code would benefit from a comprehensive comment over a const variable.

pYro_65:

[quote author=Nick Gammon link=topic=172411.msg1282312#msg1282312 date=1371520963]
What do you mean "hasn't been written"? My example stands on its own. It is written.

Your example does not include the assignment operator, therefore what are you even expecting?
[/quote]

I'm expecting to get the default assignment operator, which appears to be what I am in fact getting.

From that page:

The default version performs a memberwise copy, where each member is copied by its own copy assignment operator (which may also be programmer-declared or compiler-generated).

As the object is not an aggregate, You'll find a temporary is created of the object type using the constructor.
Then the default assignment operator copies the temporary.

Problem solved by user-defined operator!

Hi pyro_65,

In my design, I have made the object intentionally non-copyable and immutable. There is no reason to have multiple objects pointing to the same pin. Additional objects are a waste of memory. So, my design does not support it through operator= and the copy constructor. There is no good reason to change the pin. If it were a very expensive object to construct, then the answer might change. The point is moot because the object is simple to construct. The constructor is explicit because I believe that if a client can call a constructor with a single parameter and there is no legitimate reason to permit the compiler to use it to cast, then the casting should be explicitly prohibited. Nothing is gained by casting an int into a Morse.

Those are my design decisions. That is how the class will be written.

Software is typically developed in teams. The const serves to remind other team members (hypothetically) and myself in later revisions of the code, the variable should remain read only. It's a matter of being practical. True, another developer could remove the const, but in a code review system which typically shows the code reviewers diffs in the lines of code, the removal of the const would be flagged by the code review system and questioned by the code reviewers for justification. I believe that there is absolutely nothing wrong with having a const private variable. Obviously, private variables cannot be seen by the client. That's not the point. Quite obviously, the point is that private variables can be seen within the class itself.

I believe that my class is perfectly fine and well-designed.

I agree with you. Using const in a class is perfectly acceptable. In fact it makes it clear that initializing it is a once-off.

In order for global instances to work on all Arduino API compatible implementation you really should implement the 'begin' method.

Some Arduino API implementations do API initialization within 'main's 'init' method meaning it ould undo the work of 'pinMode' as specified within your constructor.

By doing it this way and only using documented API function your library should be usable across many "API compatible" boards on the market.

    ...

    Morse(uint8_t const pin)
        : _pin(pin)
    {}

    void begin()
    {
        pinMode(_pin, OUTPUT);
    }
    ...