Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use a enum class for QoS type #52

Closed

Conversation

guilhermeferreira
Copy link
Contributor

Convert to integer only in the interface with Paho MQTT C. This ensures that QoS will be used consciously. And the user will not provide an arbitrary integer instead of a QoS value.

      +-----------------+
      |                 |
      |  Paho MQTT C++  | <-- Use only QoS enumeration
      |                 |
      +-----------------+ <-- Conversion between QoS enum and int
      |                 |
      |   Paho MQTT C   | <-- Deal with interger QoS
      |                 |
      +-----------------+

The mqtt::QoS enumerators have sizeof(int) because an array of QoS might be cast to an array of integers. Thus, each element must have the same size.

@fpagliughi
Copy link
Contributor

I'm not convinced of this one. By close compatibility with the C library, and a convention across most of the Paho libraries, the QOS is just typed as an int. It is discussed by its integer values (0, 1, 2) in documentation and lists, and might be more confusing as an enumeration than just leaving it as an int.

@guilhermeferreira
Copy link
Contributor Author

guilhermeferreira commented Apr 3, 2017

My initial idea was to improve type safety. But thinking again, a QoS enumerator name might be more confusing than a simple number.

@guilhermeferreira
Copy link
Contributor Author

guilhermeferreira commented Apr 4, 2017

If you are concerned about readability, what do you think about using the numbers as enumerators? Something like this:

enum class QoS: int
{
  QoS0 = 0,	// Fire and Forget
  QoS1 = 1,	// Acknowledged delivery
  QoS2 = 2,	// Assured delivery
};

My idea here is to let the compiler warn if someone is using an arbitrary integer variable, or a constant, instead of a QoS value. The validation function (validate_qos()) works at run-time. On the other hand, this enum class method provides compile-time verification. For example:

mqtt::QoS qos = mqtt::QoS::QoS1;
// ...
qos = some_integer; // Error: compiler can spot
// ...
mqtt::message msg("payload", qos, true);
mqtt::will_options willopt("topic", "payload", qos, true);

Copy link

@miketran78727 miketran78727 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this break existing applications? If yes, please do not do this.

Convert to integer only in the interface with Paho MQTT C. This
ensures that QoS will be used consciously. And the user will not
provide an arbitrary integer instead of a QoS value.

      +-----------------+
      |                 |
      |  Paho MQTT C++  | <-- Use only QoS enumeration
      |                 |
      +-----------------+ <-- Conversion between QoS enum and int
      |                 |
      |   Paho MQTT C   | <-- Deal with interger QoS
      |                 |
      +-----------------+

The mqtt::QoS enumerators have sizeof(int) because an array of QoS
might be cast to an array of integers. Thus, each element must have
the same size.

Signed-off-by: Guilherme Maciel Ferreira <guilherme.maciel.ferreira@gmail.com>
@fpagliughi
Copy link
Contributor

I think to leave the QoS as an int for v1.0. A major point of the initial release was compatibility with the other Paho libraries, so let's stick with their lead.
Looking into the future, a v2.0 release might break away from this stricter compatibility and do a few things that are a bit more C++ centric. I would assume that might also go with C++14/17 requirement, and might possibly be an optimized, stand-alone library.

@fpagliughi fpagliughi closed this Apr 27, 2017
@guilhermeferreira guilhermeferreira deleted the fix-qos-type branch May 3, 2017 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants