ArduinoCore-API icon indicating copy to clipboard operation
ArduinoCore-API copied to clipboard

Make String::copy public

Open coogle opened this issue 8 years ago • 2 comments

I have a small feature request (and I am happy to provide the PR ), but why isn't .copy public? There is a use-case here which makes a lot of sense -- specifically when you are given a pointer to a char* that is not null-terminated (and instead are provided a length).

This is exactly how the PubSubClient MQTT library works. The topic callback provides you a byte *payload and length, but because it's byte * it's not null-terminated.

Currently you have to do this (or something like it) to get a String object:

void mqttCallback(char *topic, byte *payload, unsigned int length) {
    String action;
    char *payloadStr;

    payloadStr = (char *)malloc(length + 1);
    memcpy(payloadStr, payload, length);
    payloadStr[length] = 0x0;
    action = String(payloadStr);
    free(payloadStr);
}

When what would make a lot more sense (since String doesn't provide a constructor for this) would be:

void mqttCallback(char *topic, byte *payload, unsigned int length) {
    String action;
    action.copy((char *)payload, length);
}

coogle avatar Jul 24 '17 17:07 coogle

I think your usecase is covered by arduino/Arduino#1936 (it adds a String(ptr, len) constructor), which unfortunately has not been merged yet...

matthijskooijman avatar Jul 25 '17 08:07 matthijskooijman

I agree it is, but that PR hasn't been merged for a reason! :)

There isn't really any good reason IMO that copy() isn't public already. Trying to change the constructor when it comes to overloading is going to face potential BC breaks of old code. Making copy() public makes sense from an API perspective, it makes sense from a non-BC-break perspective, and it makes sense from a "I shouldn't have to use malloc to create a string here" perspective.

coogle avatar Jul 25 '17 16:07 coogle