Mqtt5 GA API Review #575
Mqtt5 GA API Review #575
Conversation
|
There are some breaking changes (e.g., |
bin/mqtt5_canary/main.cpp
Outdated
| @@ -450,7 +450,7 @@ static int s_AwsMqtt5CanaryOperationUnsubscribeBad(struct AwsMqtt5CanaryTestClie | |||
| unsubscription, [testClient](int, std::shared_ptr<Mqtt5::UnSubAckPacket> packet) { | |||
| if (packet == nullptr) | |||
| return; | |||
| if (packet->getReasonCodes()[0] == AWS_MQTT5_UARC_SUCCESS) | |||
| if (packet->getReasonCodes()[0] == UnSubAckReasonCode::AWS_MQTT5_UARC_SUCCESS) | |||
There was a problem hiding this comment.
This isn't what I had in mind for the enums. Did the original proposal not work?
There was a problem hiding this comment.
As I'm creating the enum using the same value name in aws-c-mqtt (UnSubAckReasonCode:: AWS_MQTT5_UARC_SUCCESS v.s. aws_mqtt5_unsuback_reason_code::AWS_MQTT5_UARC_SUCCESS), the compiler would report an ambiguity error. I had to add the namespace to avoid the error.
One way to fix it is using a different enum value name in CPP. For example: UnSubAckReasonCode ::AWS_MQTT5_UARC_SUCCESS -> UnSubAckReasonCode::SUCCESS. In this case, we could still directly pass AWS_MQTT5_UARC_SUCCESS as the enum would be converted implicitly. However, it would break the user who was using UnSubAckReasonCode::AWS_MQTT5_UARC_SUCCESS.
I'm not sure if there is a better way to fix it.
There was a problem hiding this comment.
That was what I intended:
enum class UnsubackReasonCode {
Success = AWS_MQTT5_UARC_SUCCESS,
... etc...
};
There was a problem hiding this comment.
-
It seems that enum class would not be able to directly convert between int and enum value. Using enum class would definitely breaks the customer.
-
If we removed AWS_MQTT5_UARC_SUCCESS and rename it to
Success, then it would break the customer if they already use the enum with the namespace like following pattern:
UnSubAckReasonCode reasonCode = UnSubAckReasonCode::AWS_MQTT5_UARC_SUCCESSThere was a problem hiding this comment.
You could double the entries with the palatable names, but I'm not sure the extra effort is worth it.
Forcing people to use the name mangling prefix is crummy though.
Issue #, if available:
Description of changes:
Verified in aws/aws-iot-device-sdk-cpp-v2#659 that the change would not break current cpp v2.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.