Вопрос
Недавно я написал следующее splitv()
функция без с помощью strtok
. Есть ли способ сделать его короче и эффективнее?
Описание
Функция вычисляет index-th
элемент строки str
разделен на delim
. Элемент записывается в buff
и 0
возвращается. При возникновении ошибки индекса любой non 0
значение возвращается. Эта функция была вдохновлена функцией разделения Python: buff = str.split(delim)[index]
, но я думаю, что это работает слишком много и может быть улучшено.
int splitv(char buff[], char str[], char delim[], int index) {
// returns 0 if an element is written. Else non 0.
char *start = str, *end;
int i = 0, buff_i = 0;
// check if delim not in str
if ((end = strstr(str, delim)) == NULL) {
if (index == 0) strcpy(buff, str);
return index;
}
// delim is in str
for (i = 0; i < index; i++) {
start = end + strlen(delim);
end = strstr(start, delim);
if (end == NULL) {
// reached the last element
if (i == index - 1) {
end = str + strlen(str);
break;
}
// index error: index >= elements_count
return -1;
}
}
// copy element to buffer
while (start != end) {
buff[buff_i] = *start;
start++;
buff_i++;
}
buff[buff_i] = ' ';
return 0;
}
Заранее спасибо!
2 ответа
Прежде чем говорить о повышении эффективности вашей функции, давайте поговорим о безопасности:
- Вы никогда не проверяете правильность параметров вашей функции.
- Вы не знаете, насколько велик
buff
является.
Для первого выпуска, если str
или delim
имеет значение null, передавая их непосредственно в strstr
вызовет segfault и приведет к сбою вашей программы.
Вторая проблема хуже: если buff
меньше, чем str
, ваша функция может в конечном итоге засорять память при попытке скопировать из str
к buff
.
Это неопределенное поведение, и результаты непредсказуемы.
Теперь несколько советов по эффективности:
Рассчитайте длину строки только один раз.
посколькуstrlen
работает, перебирая строку в поисках терминатора ‘ 0’, более эффективно вызвать его только один раз в начале вашей функции и сохранить результат в переменной.
Это также может позволить вам реализовать простой ярлык для крайних случаев, например, когдаstr
илиdelimiter
пустые строки, или разделитель длиннееstr
.Тебе не нужно свое
while
петля в конце иbuff_i
переменная.
Вы можете просто использоватьmemcpy
илиstrncpy
чтобы скопировать элемент в буфер.memcpy
будет немного более эффективным, поскольку он не проверяет ‘ 0’, аstrncpy
делает.Поскольку ваша логика такова, что если разделитель никогда не найден, но запрошенный элемент равен 0, результатом будет «успех», вам не нужно обрабатывать это как отдельный пограничный случай.
Это означает, что вы можете избавиться от первогоif
блок и обрабатывать все в одном цикле.
Вы также можете избавиться от специальных проверок вif
цикл и просто сосредоточьтесь на поиске.
Следующий пример кода не включает предлагаемые проверки действительности:
int i;
char *start = str;
char *end = start;
ssize_t delim_len = strlen(delim);
ssize_t copy_len;
for (i = 0; i <= index && end != NULL; i++) {
end = strstr(start, delim);
if (end != NULL && i != index) start = end + delim_len;
}
/* element not found */
if (i != index) return index;
if (end == NULL) {
copy_len = strlen(start);
} else {
copy_len = (ssize_t)(end - start);
}
memcpy(buff, start, copy_len);
buff[copy_len] = ' ';
return 0;
int splitv(char buff[], char str[], char delim[], int index) { // returns 0 if an element is written. Else non 0.
Несколько моментов об этом интерфейсе:
- Не должен
delim
быть указателем наconst char
? Я думаю, что функция не должна его изменять. Точно так же, если мы можем избежать изменения входной строки (что является одним из основных преимуществ отказа от использованияstrtok()
), то тоже примите это как указатель на константу. - Пожалуйста, примите аргумент, чтобы указать вместимость
buff()
чтобы мы могли писать код без переполнения буфера! index
должно быть без знака, поскольку отрицательные значения здесь не имеют смысла.- Мы могли бы сделать возвращаемое значение более полезным, вернув длину совпадения (или отрицательное значение в случае неудачи). Как интерфейс к
snprintf()
, это очень полезно, если буфер слишком мал. Я бы даже согласилсяNULL, 0
для аргументов буфера таким же образом. - Более традиционно аргументы указателя символа записываются как
char *arg
чем с синтаксисом неопределенного массива. С точки зрения компилятора, конечно, они идентичны, но соблюдение соглашений облегчает чтение кода другим программистам.
Посмотрим, как мы будем использовать существующий интерфейс:
#include <stdio.h>
int main(void)
{
char list[] = "Alice, Bob, Charlie, Don";
char delim[] = ", ";
char buff[10]; /* is that enough?? */
if (splitv(buff, list, delim, 2)) {
fputs("Couldn't split the string.n", stderr);
return 1;
}
printf("Found %sn", buff);
}
Посмотрите, насколько проще новый интерфейс:
int splitv(char *buff, size_t buff_len,
const char *str, const char *delim,
unsigned int index);
#include <stdio.h>
int main(void)
{
const char *list = "Alice, Bob, Charlie, Don";
char buff[10];
int len = splitv(buff, sizeof buff, list, ", ", 2);
if (len < 0) {
fputs("Couldn't split the string.n", stderr);
return 1;
}
if ((size_t)len >= sizeof buff) {
fputs("Substring too long.n", stderr);
return 1;
}
printf("Found %sn", buff);
}
Нам не нужно угадывать, достаточно ли велик наш буфер (и адаптивный код может использовать результат для выделения достаточно большого буфера), и мы можем передавать строковые литералы в качестве аргументов.
const char *start = str, *end; int i = 0, buff_i = 0;
Предпочитаю объявлять по одной переменной в каждой строке и предпочитаю объявлять, где вы также можете инициализировать:
const char *start = str;
// check if delim not in str
const char *end = strstr(str, delim);
if (!end) {
for (int i = 0; i < index; ++i) {
У нас есть два бита кода, которые делают то же самое, когда мы достигаем последнего элемента. Это может помочь переосмыслить, что делает этот код:
- Пропустить ноль или более подстрок, оканчивающихся на
delim
. - Найдите следующее вхождение
delim
, или конец строки. - Скопируйте подстроку.
Вот упрощенная реализация, которая соответствует предложенному мной интерфейсу и структурирована в соответствии с этими шагами:
#include <stdint.h>
#include <string.h>
int splitv(char *buff, size_t buff_len,
const char *str, const char *delim,
unsigned int index)
{
/* returns length of the substring if it exists */
/* returns negative on failure */
/* useful constant */
const size_t delim_len = strlen(delim);
while (index--) {
/* advance past next delimiter */
const char *next = strstr(str, delim);
if (!next) {
/* not enough delimiters */
return -1;
}
str = next + delim_len;
}
/* okay, we've found the right substring; now find its end */
const char *end = strstr(str, delim);
if (!end) {
end = str + strlen(str);
}
/* decide how much to copy, and write it */
size_t len = (size_t)(end - str);
if (buff) {
size_t write_size = len < buff_len ? len : buff_len - 1;
/* write the output */
memcpy(buff, str, write_size);
buff[write_size] = 0;
}
/* return the substring length (not including null terminator) */
return (int)len;
}
Обратите внимание, что я поднял strlen(delim)
в константу, поскольку мы знаем, что это не нужно повторно вычислять каждый раз, когда проходит цикл. Я также использовал стандартные memcpy
функция вместо ручного цикла для записи в buff
.
- 1
Помимо создания
index
без подписи, это должен бытьsize_t
, так как эта функция не будет работать на большинстве (всех?) 64-битных системах с более чем 4Б вхождений— фары0123
- 1
Да, я так думаю. Фактически, на ранней итерации я делал именно это, и я не уверен, почему я изменил это обратно. И
int
поскольку возвращаемый тип тоже неверен (вероятно, должен бытьsize_t
, с участием(size_t)-1
используется в качестве дозора ошибок. Могу ли я утверждать, что мой код — это не законченный элемент, а всего лишь шаг вперед?— Тоби Спейт