Комментарии 133
Я так понимаю, функция csv_proc_fetch_value копирует строку из temp в out_buff. Почему бы сразу в out_buff не писать в таком случае?
Да. Всё верно.
bool csv_proc_fetch_value(CsvFsm_t* CsvFsm, uint32_t cur_index) {
bool res = false;
if(CsvFsm->fetch_index == cur_index) {
uint32_t val_size = 0;
val_size = strlen(CsvFsm->temp);
LOG_DEBUG(CSV, "Fetch [%s] Len %u", CsvFsm->temp, val_size);
if(val_size < CsvFsm->out_size) {
LOG_DEBUG(CSV, "Fetch Ok");
strcpy(CsvFsm->out_buff, CsvFsm->temp);
} else {
LOG_ERROR(CSV, "LackOfSpase ValueSize:%u TotalSize:%u", val_size, CsvFsm->out_size);
}
res = true;
} else {
LOG_DEBUG(CSV, "Skip Cur:%u Need:%u", cur_index, CsvFsm->fetch_index);
}
return res;
}
меня больше интересует - зачем копировать в temp всё подряд :)
csv_proc_fetch_value хотя бы копирует только нужное значение из temp, а вот в temp пишется всё подряд, любое встреченное значение, вне зависимости от строки и индекса. Потом уже, при совпадении строки и индекса - нужное значение из него будет копироваться в out_buff. Почему просто не запомнить указатель на начало значения, и избавиться от temp полностью (там еще и размер этого буфера не проверяется - легко устроить buffer overflow), я не могу понять. Единственная гипотеза - это было скопировано из настоящего парсера, где обрабатывались кавычки, и временный буфер использовался для нормализации двойных кавычек. Но врядли.
2--В передаваемом тексте не должно быть самого символа разделителя как данных. Иначе конечный автомат заклинит.
В CSV вполне можно передавать поля с разделителем. Для этого поля заключаются в кавычки, как написано здесь https://en.wikipedia.org/wiki/Comma-separated_values
Я правильно понимаю, что если мне надо больше двух значений извлечь, то придётся вызывать csv_parse_text n раз?
Задача распасить csv уже решена тысячи раз. Чем ваше решение лучше любого другого из тех что есть на Гитхабе? Чем хуже понятно. 0 звезд Гитхаба, 0 истории использования. Вероятность критических ошибок больше чем в любом массовом решении написанном 10 лет назад.
Миру точно нужно еще одно решение уже давно решенной задачи?
Чем ваше решение лучше любого другого из тех что есть на Гитхабе?
В моем решении нет нужды в динамическом выделении памяти, что особенно важно для программирования микроконтроллеров.
В добавок к этому я представил не только код, но и документацию.
Ваш код очень усложнён, вы зачем-то использовали конечный автомат и копирование, которое может быть не всегда нужно. Плюс для получения нескольких столбцов надо одну и ту же строку несколько раз сначала парсить, что неэффективно. Я за минут 30 накатал более красивый вариант:
csv.h:
#pragma once
#include <stdint.h>
#include <stdbool.h>
typedef struct {
char *column;
uint32_t len;
} csv_column_t;
typedef struct {
char *line;
uint32_t line_size;
uint32_t cur_pos;
char sep;
char repl_char;
} csv_parser_t;
void init_parser(csv_parser_t *parser, char *line, char sep);
bool csv_next_column(csv_parser_t *parser, csv_column_t *column);csv.c:
#include "csv.h"
#include <string.h>
void init_parser(csv_parser_t *parser, char *line, char sep) {
parser->line = line;
parser->line_size = strlen(line);
parser->cur_pos = 0;
parser->sep = sep;
}
bool csv_next_column(csv_parser_t *parser, csv_column_t *column) {
if(parser->cur_pos != 0) {
// Восстанавливаем разделитель в строке
parser->line[parser->cur_pos] = parser->repl_char;
parser->cur_pos++;
}
column->column = &parser->line[parser->cur_pos];
column->len = 0;
uint32_t start_pos = parser->cur_pos;
for(; parser->cur_pos < parser->line_size; parser->cur_pos++) {
// Если символ не является разделителем, идём дальше
if(parser->line[parser->cur_pos] != parser->sep &&
parser->line[parser->cur_pos] != '\n' &&
parser->line[parser->cur_pos] != '\r')
continue;
// Временно заменяем разделитель на ноль
column->len = parser->cur_pos - start_pos;
parser->repl_char = parser->line[parser->cur_pos];
parser->line[parser->cur_pos] = 0;
return true;
}
column->len = parser->cur_pos - start_pos;
if(column->len != 0) {
return true;
} else if(parser->cur_pos > 0) {
parser->line[parser->cur_pos - 1] = parser->repl_char;
}
return false;
}Вот как с этим работать:
test.csv:
Name,Surname,Age,Gender
John,Week,36,male
Tesla,Killer,11,male
Jane,Doe,34,female
Liza,Silver,26,femalemain.c:
#include <stdio.h>
#include <stdint.h>
#include <stdbool.h>
#include "csv.h"
// Хотим вывести только имя и пол
int main(void) {
char buf[4096];
while(fgets(buf, sizeof(buf), stdin)) {
csv_parser_t parser;
init_parser(&parser, buf, ',');
csv_column_t column;
for(uint32_t i = 0; ; i++) {
// если res = false, значит данных в сроке больше нет
bool res = csv_next_column(&parser, &column);
if(!res)
break;
if(i == 0 || i == 3) {
// делаем что-то со столбцом
}
}
}
}Ваш код не соответствует требуемому API
Ваш код очень усложнён, вы зачем-то использовали конечный автомат и копирование, которое может быть не всегда нужно.
Дизайн на основе FSM как раз упрощает код, ибо все понимают, что есть такой паттерн, как он работает и как его использовать.
Тем более FSM надо использовать согласно ISO26262.
Мой код куда проще и понятнее вашего при этом ещё и эффективнее
Покажите лучше отчет после прогона модульных тестов для вашего кода. Вот набор тест-case(ов)
#include "test_csv.h"
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include "csv.h"
#include "log.h"
#include "unit_test_check.h"
bool test_csv_cnt(void){
LOG_INFO(TEST, "%s():", __FUNCTION__);
bool res = true;
set_log_level(CSV, LOG_LEVEL_DEBUG);
EXPECT_EQ(1, csv_cnt("", ';') );
EXPECT_EQ(2, csv_cnt(";", ';') );
EXPECT_EQ(1, csv_cnt("a", ';') );
EXPECT_EQ(3, csv_cnt(";;", ';') );
EXPECT_EQ(4, csv_cnt(",,,", ',') );
EXPECT_EQ(1, csv_cnt("4452", ';') );
EXPECT_EQ(6, csv_cnt("7354, 1105.000,20.75, 16:07:44, 14/7/2023, 1517952720", ',') );
EXPECT_EQ(3, csv_cnt("ll wm8731 debug;ll i2c debug;tsr 127", ';') );
set_log_level(CSV,LOG_LEVEL_INFO);
return res;
}
bool test_csv_parse_text(void){
LOG_INFO(TEST, "%s():", __FUNCTION__);
bool res = true;
set_log_level(CSV, LOG_LEVEL_DEBUG);
char sub_text[80] = "";
memset(sub_text, 0, sizeof(sub_text));
EXPECT_TRUE( csv_parse_text("",';', 0, sub_text, sizeof(sub_text)));
EXPECT_STREQ("", sub_text);
EXPECT_TRUE( csv_parse_text("h",';', 0, sub_text, sizeof(sub_text)));
EXPECT_STREQ("h", sub_text);
EXPECT_TRUE( csv_parse_text(";;c;;",';', 2, sub_text, sizeof(sub_text)));
EXPECT_STREQ("c", sub_text);
EXPECT_TRUE( csv_parse_text("ll wm8731 debug;ll i2c debug;tsr 127",';', 0, sub_text, sizeof(sub_text)) );
EXPECT_STREQ("ll wm8731 debug", sub_text);
EXPECT_TRUE( csv_parse_text("ll wm8731 debug;ll i2c debug;tsr 127",';', 1, sub_text, sizeof(sub_text)) );
EXPECT_STREQ("ll i2c debug", sub_text);
EXPECT_TRUE( csv_parse_text("ll wm8731 debug;ll i2c debug;tsr 127",';', 2, sub_text, sizeof(sub_text)) );
EXPECT_STREQ("tsr 127", sub_text);
set_log_level(CSV, LOG_LEVEL_INFO);
return res;
}
С удовольствием, дайте мне тесты, я вам предоставлю
#include "test_csv.h"
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include "csv.h"
#include "log.h"
#include "unit_test_check.h"
bool test_csv_cnt(void){
LOG_INFO(TEST, "%s():", __FUNCTION__);
bool res = true;
set_log_level(CSV, LOG_LEVEL_DEBUG);
EXPECT_EQ(1, csv_cnt("", ';') );
EXPECT_EQ(2, csv_cnt(";", ';') );
EXPECT_EQ(1, csv_cnt("a", ';') );
EXPECT_EQ(3, csv_cnt(";;", ';') );
EXPECT_EQ(4, csv_cnt(",,,", ',') );
EXPECT_EQ(1, csv_cnt("4452", ';') );
EXPECT_EQ(6, csv_cnt("7354, 1105.000,20.75, 16:07:44, 14/7/2023, 1517952720", ',') );
EXPECT_EQ(3, csv_cnt("ll wm8731 debug;ll i2c debug;tsr 127", ';') );
set_log_level(CSV,LOG_LEVEL_INFO);
return res;
}
bool test_csv_parse_text(void){
LOG_INFO(TEST, "%s():", __FUNCTION__);
bool res = true;
set_log_level(CSV, LOG_LEVEL_DEBUG);
char sub_text[80] = "";
memset(sub_text, 0, sizeof(sub_text));
EXPECT_TRUE( csv_parse_text("",';', 0, sub_text, sizeof(sub_text)));
EXPECT_STREQ("", sub_text);
EXPECT_TRUE( csv_parse_text("h",';', 0, sub_text, sizeof(sub_text)));
EXPECT_STREQ("h", sub_text);
EXPECT_TRUE( csv_parse_text(";;c;;",';', 2, sub_text, sizeof(sub_text)));
EXPECT_STREQ("c", sub_text);
EXPECT_TRUE( csv_parse_text("ll wm8731 debug;ll i2c debug;tsr 127",';', 0, sub_text, sizeof(sub_text)) );
EXPECT_STREQ("ll wm8731 debug", sub_text);
EXPECT_TRUE( csv_parse_text("ll wm8731 debug;ll i2c debug;tsr 127",';', 1, sub_text, sizeof(sub_text)) );
EXPECT_STREQ("ll i2c debug", sub_text);
EXPECT_TRUE( csv_parse_text("ll wm8731 debug;ll i2c debug;tsr 127",';', 2, sub_text, sizeof(sub_text)) );
EXPECT_STREQ("tsr 127", sub_text);
set_log_level(CSV, LOG_LEVEL_INFO);
return res;
}Кое-что моя реализация не может - работать с read only строками (не думаю, что это прямо проблема, потому что вы всё равно из внешнего источника csv получаете). За 30 минут сделать не вышло, тут вы меня подловили, но я развлёкся зато. Вот исправленная реализация, проходящая все ваши тесты (плюс бонус - я реализовал ваше апи через своё).
csv.h:
#pragma once
#include <stdint.h>
#include <stdbool.h>
typedef struct {
char *column;
uint32_t len;
} csv_column_t;
typedef struct {
char *line;
uint32_t line_size;
uint32_t cur_pos;
char sep;
char repl_char;
bool is_first;
bool is_last;
} csv_parser_t;
void init_parser(csv_parser_t *parser, char *line, char sep);
bool csv_next_column(csv_parser_t *parser, csv_column_t *column);
void csv_restore_line(csv_parser_t *parser);csv.c:
#include "csv.h"
#include <string.h>
void init_parser(csv_parser_t *parser, char *line, char sep) {
parser->line = line;
parser->line_size = strlen(line);
parser->cur_pos = 0;
parser->repl_char = 0;
parser->is_first = true;
parser->is_last = false;
parser->sep = sep;
}
bool csv_next_column(csv_parser_t *parser, csv_column_t *column) {
if(!parser->is_first) {
// Восстанавливаем разделитель в строке
parser->line[parser->cur_pos] = parser->repl_char;
parser->repl_char = 0;
parser->cur_pos++;
} else {
parser->is_first = false;
}
column->column = &parser->line[parser->cur_pos];
column->len = 0;
uint32_t start_pos = parser->cur_pos;
for(; parser->cur_pos < parser->line_size; parser->cur_pos++) {
// Если символ не является разделителем, идём дальше
if(parser->line[parser->cur_pos] != parser->sep &&
parser->line[parser->cur_pos] != '\n' &&
parser->line[parser->cur_pos] != '\r')
{
parser->is_last = true;
continue;
}
// Временно заменяем разделитель на ноль
column->len = parser->cur_pos - start_pos;
parser->repl_char = parser->line[parser->cur_pos];
parser->line[parser->cur_pos] = 0;
return true;
}
column->len = parser->cur_pos - start_pos;
if(column->len != 0)
return true;
if(parser->is_last) {
if(parser->cur_pos > 0)
parser->line[parser->cur_pos - 1] = parser->repl_char;
return false;
} else {
parser->is_last = true;
return true;
}
}
void csv_restore_line(csv_parser_t *parser) {
if(!parser->is_first) {
parser->line[parser->cur_pos] = parser->repl_char;
}
}test.c:
#include <stdint.h>
#include <stdio.h>
#include <string.h>
#include <assert.h>
#include "csv.h"
uint32_t csv_cnt(char *line, char sep) {
csv_parser_t parser;
init_parser(&parser, line, sep);
uint32_t count;
for(count = 0; ; count++) {
csv_column_t column;
if(!csv_next_column(&parser, &column))
break;
}
return count;
}
#define CNT_TEST(str, sep, eq) do { \
char buf[] = str; \
assert(csv_cnt(buf, sep) == eq); \
assert(!strcmp(buf, str)); \
} while(0)
void test_csv_cnt(void) {
CNT_TEST("", ';', 1);
CNT_TEST(";", ';', 2);
CNT_TEST("a", ';', 1);
CNT_TEST(";;", ';', 3);
CNT_TEST(",,,", ',', 4);
CNT_TEST("4452", ';', 1);
CNT_TEST("7354, 1105.000,20.75, 16:07:44, 14/7/2023, 1517952720", ',', 6);
CNT_TEST("ll wm8731 debug;ll i2c debug;tsr 127", ';', 3);
}
bool csv_parse_text(char *line, char sep, uint32_t index, char *out, uint32_t size) {
csv_parser_t parser;
init_parser(&parser, line, sep);
for(uint32_t i = 0; ; i++) {
csv_column_t column;
if(!csv_next_column(&parser, &column))
return false;
if(i != index)
continue;
if(size <= column.len)
return false;
memcpy(out, column.column, column.len + 1);
csv_restore_line(&parser);
return true;
}
}
#define PARSE_TEST(str, sep, idx, out_buf, res, column) \
do { \
char buf[] = str; \
assert(csv_parse_text(buf, sep, idx, out_buf, sizeof(out_buf)) == res); \
assert(!strcmp(out_buf, column)); \
assert(!strcmp(str, buf)); \
} while(0)
void test_csv_parse_text(void) {
char column[80];
PARSE_TEST("", ';', 0, column, true, "");
PARSE_TEST("h", ';', 0, column, true, "h");
PARSE_TEST(";;c;;", ';', 2, column, true, "c");
PARSE_TEST("ll wm8731 debug;ll i2c debug;tsr 127", ';', 0, column, true, "ll wm8731 debug");
PARSE_TEST("ll wm8731 debug;ll i2c debug;tsr 127", ';', 1, column, true, "ll i2c debug");
PARSE_TEST("ll wm8731 debug;ll i2c debug;tsr 127", ';', 2, column, true, "tsr 127");
}
int main(void) {
test_csv_cnt();
test_csv_parse_text();
}Считаю что Вам понравится вот этот текст
51 Атрибут Хорошего С-кода (Хартия Си программистов)
https://habr.com/ru/articles/679256/
Возьмем первое найденное решение на Гитхабе у которого звездочек побольше.
https://github.com/rgamble/libcsv
Сотня звездочек есть. Работает вероятно быстрее вашего. Коду уже много-много лет. Вероятность что в нем есть ошибки стремится к нулю. Код выглядит проще чем у вас. Документация тоже лучше чем у вас.
И зачем? Не, ну если это лаба студента я понимаю. Таких лабораторок тоже тысячи в год пишутся.
В этом коде есть множественный return. Это запрещено стандартом ISO26262
Рекомендую почитать этот текст
ISO 26262-6 разбор документа (или как писать безопасный софт)
https://habr.com/ru/articles/757216/
Вы никогда не слышали про концепцию fail fast? Функции проверяющие что-то в начале работы и быстро выходящие если это не так читаются заметно легче.
А вот монстрообразные вложенные ифы чтобы дотащить все ветки до вашего единственного return читаются очень тяжело. Я бы на код ревью зарубил такое.
Концепция сама по себе неплоха, но рискованная т.к. к моменту когда "что-то пошло не так и нужно выйти" функция уже может сделать что-то такое, что требует подчистки за собой. И для этого существует более безопасная концепция "единой точки выхода". К сожалению, в С, красиво не реализуется иначе чем через GOTO (в С++ это реализуется через исключения).
В некоторых языках для это реализован специальный блок on-exit [err_flag], в который попадаем после return (и можем там изменить возвращаемое значение) или при возникновении ошибки-исключения (тогда, если указан err_flag, он будет взведен.
Есть языки, поддерживающие "сабрутины" (subroutine) - подпрограммы (как в бейсике) внутри функций. Там можно реализовывать единую точку выхода через такую сабрутину - вместо return вызывается она, там выполняются все необходимые действия и после этого уже делается return.
В любом случае, единая точка выхода является более безопасной реализацией.
Как концепция "единой точки выхода" защитит меня от забывчивости забыть освободить ресурсы?
От склероза не защитит ничего. Тут только голову электричеством лечить.
Но концепция единой точки выхода позволит сделать код более компактным и понятным в ситуации, когда перед return нужно сделать что-то еще (даже дублировать 3-4 строки кода перед каждым return уже не только накладно, но еще повышает риск в 2-х местах поставить их, а в третьем забыть или пропустить по невнимательности).
Так же в случае модификации кода когда перед выходом нужно добавить какое-то действие снижается риск того, что где-то его добавите, а где-то пропустите.
Вы описали ситуацию, где надо перед возвратом что-то делать, но в нашем же случае это не так. Зачем тогда слепо следовать правилу одной точки выхода? Если надо что-то делать перед возвратом, соглашусь должна быть одна точка выхода, если нет, то не вижу проблемы в множественных точках возврата.
Множественный return запрещен стандартом ISO26262
Рекомендую почитать этот текст
ISO 26262-6 разбор документа (или как писать безопасный софт)https://habr.com/ru/articles/757216/
Это говорит только о том, что стандарты должны меняться, когда становится понятно, что некоторые вещи в них необоснованны и странны. Но если вам строго надо следовать стандарту, тут не попишешь, придётся одну точку выхода делать.
Это еще может говорить о том, что если для нас что-то неочевидно или интуитивно непонятно, то это совершенно необязательно неправильно.
Приведите пример, где множественный return делает код небезопасным, и при этом нам не нужно ничего делать при выходе из функции. Если же нам надо что-то делать при выходе из функции, чем плох goto? Какую угрозу безопасности он несёт в таком случае?
У Эппла был хороший баг, так и называется в народе - goto fail.
Без goto имхо лучше стараться обходится, а вот запрет множественного return и тем более continue - это безумие.
В софте, от которого зависят жизни, наверное стоит. Правда, мне в голову только такой вариант приходит, чтобы не нагромождать вложенные ифы:
int func(void) {
int err = action_1();
if(err != 0)
return cleanup_func(err, arg1, arg2, ...);
// какой-то код
err = action_2();
if(err != 0)
return cleanup_func(err, arg1, arg2, ...);
//какой-то код
return 0;
}Ну или везде использовать фигурные скобки, тогда ситуация, которая была у apple с меньшей вероятностью произойдёт.
В софте, от которого зависят жизни, наверное стоит
Уже есть куча статей про автомобильный софт. Там страх и ужас. Типичный хайлоад сервис с девятками надежности написан лучше.
Зато в автомобильном софте хорошо умеют подстилать соломки. Вотчдоги на все, вообще на все. Действие по умолчанию когда все сломалось нормальное. Перезагружается все само, быстро и инициализируется нормальным состоянием. И все такое. Тут типичному хайлоад сервису с девятками стоило бы поучиться.
Тут уже складывается ощущение, что "любой костыль, лишь бы сохранить несколько точке выхода".
Это хорошо когда то, что подлежит зачистке, лежит в глобальной области видимости и доступно из cleanup_func, а не в локальной области func.
А что бы будете делать, если у вас в процессе выполнения func открывается пара-тройка файлов (которые нужно закрыть на выходе), блокируются какие-то записи (которые нужно разблокировать перед выходом), динамически выделяется временная память, которую нужно освободить?
При этом код
int func(void) {
int err = some_action_1();
if(err == 0) err = some_action_2();
if(err == 0) err = some_action_3();
// освобождаем локально выделенные ресурсы
//...
// если была ошибка и нужен откат
if (err != 0) rollback_func();
return err;
}не выглядит такой уж "лапшой" и не содержит многоуровневой вложенности. Но при этом просто лаконичнее.
А что бы будете делать, если у вас в процессе выполнения func открывается пара-тройка файлов (которые нужно закрыть на выходе), блокируются какие-то записи (которые нужно разблокировать перед выходом), динамически выделяется временная память, которую нужно освободить?
Деструкторы в C++ приблизительно это и должны делать же. Если очень нужно именно C - не забываем инициализировать; подпираем костылем:
if (file1) {fclose(file1); file1 = NULL;};
if (memory) {free(memory); memory=NULL;};Деструкторы в C++ приблизительно это и должны делать же
В С++ единая точка входа реализуется исключениями. Просто и красиво - возникла ошибка - кидаем исключение и попадаем в ту самую "единую точку выхода".
Если очень нужно именно C - не забываем инициализировать; подпираем костылем
И так 100500 раз на каждый return? О чем и говорю - лютый костылинг и дикое дублирование кода. Он просто становится замусоренным и слабочитаемым.
О чем, собственно, и речь - множественные return удовлетворительно работают в очень простых и небольших функциях. И всегда есть риск того, что если потребуется усложнить логику функции, то сразу возникнет куча проблем.
Единая точка выхода более универсальна - я показывал как это работает на простых функциях и точно также это будет работать и на более сложных. Т.е. этот подход более устойчив к изменениям в сторону усложнения логики.
Мне ни в коей мере не хочется сводит все это к холивару и поливанию друг друга разными дурно пахнущими жидкими субстанциями. Просто высказал свою точку зрения, сформированную многолетним опытом разработки в разных областях и на разных языках.
О чем, собственно, и речь - множественные return удовлетворительно работают в очень простых и небольших функциях.
Да. Есть даже паттерн такой - простые небольшие функции ;)
Уж лучше goto, чем исключения, как по мне. goto ведёт всегда в одну точку причём в той же функции, а исключение ведёт к месту обработки. Забыл обработать нужный класс исключений в своей функции, тебя выбросит в другую, а в худшем случае вся программа упадёт
Если функция большая, её, конечно, нужно разбить на несколько простых. Но если она маленькая, то ради единой точки выхода, правда, нужно создавать ещё три функции? Это очень усложняет чтение, потому что вместо одной функции на 20 строк, у вас одна функция, в которой вызывается три других, и вам надо посмотреть, что делает каждая. Да это лапшой не назовёшь, но воспринимается такой код сложнее
Ну, во-первых, лучше везде придерживаться одного стиля, а единая точка выхода более универсальна.
Во-вторых, то что вы сейчас написали функцию, где перед выходом ничего не надо делать, совершенно не гарантирует того, что через месяц вам ее не придется модифицировать до состояния когда перед выходом что-то надо будет сделать. Зачем самому себе закладывать потенциальные сложности?
Как пример из языка, где поддерживаются оба механизма:
on-exit;
if curOpnd;
exec sql close curRDKCHK1Clients;
curOpnd = *off;
endif;
end-proc;Тут перед выходом проверяется открыт ли sql курсор и если его успели открыть до выхода - нужно закрыть.
В это блок автоматически попадем при return где бы он ни был.
Второй вариант:
BegSr srExit;
if error <> *blanks;
needLogError = *on;
MQlogMSG = %trim(Message_Text(error));
RAMSGTYPE = 'CAFZ01ERR';
clear rtnCode;
endif;
return;
endsr;Проверяется была ли ошибка и если была, выполняются некоторые действия.
Тут вместо return делаем exsr srExit;
В любой реализации, нам не надо для каждого выхода прописывать одни и те же действия. И если потребуется модифицировать код так, что список действий на выходе потребует расширения или изменения, сделать это надо будет в одном месте а не шарахаться по всему коду в поисках "а где там у нас еще точки выхода были" с риском что-то проглядеть.
@SpiderEkb Подписываюсь под каждым Вашим словом.
Ну сделайте рядом метод clearAfterMyFunc. Будет у вас единая точка очистки ресурсов.
Вы никогда не слышали про концепцию fail fast? Функции проверяющие что-то в начале работы и быстро выходящие если это не так читаются заметно легче.
Что Вы мне то это объясняете?
Скажите это чиновникам, которые составили стандарт ISO-26262.
https://habr.com/ru/articles/757216/
Чиновники часто не очень понимают что делают. Это нормально.
Если где-то вместо нормальных правил написания кода, это любые стайлгайды от любой крупной продуктовой конторы, заставляют использовать писанину от чиновников то бежать оттуда надо.
Вообще то вся мировая разработка Automotive работает по ISO26262.
Ну-ну. Вы сами сертификацию проходили?
Automotive компилятор С (например GHS) даже не соберет код, где в функции 2 return(а).
Там сертификация встроена прямо в ядро Си-компилятора.
А goto использовать разрешается?
А goto использовать разрешается?
запрещено
Один return без goto приводит к такой лапше из вложенных ифов, что приводит к увеличению вероятности ошибки и усложнению понимания работы кода. Странный стандарт
Вот вы бы вместо минуса привели бы лучше пример, который доказывает мою неправоту. Доупстим есть функция
int func(void) {
int err = some_action_1();
if(err != 0) {
// обрабатываем ошибку,
// возвращаем err
return err;
}
// делаем что-то полезное,
// потом снова вызываем функцию,
// которая может вернуть ошибку
err = some_action_2();
if(err != 0) {
// обрабатываем ошибку,
// возвращаем err
return err;
}
// делаем что-то ещё, возвращаем 0
return 0;
}Перепишите её так, чтобы точка выхода была одна, и код не стал лапшой.
int func(void) {
int err = some_action_1();
if(err == 0) err = some_action_2();
if(err == 0) err = some_action_3();
// делаем что-то ещё
// ...
return err;
}А если нужна именно последовательность, как я описал. То есть после some_action_1 должен идти какой-то код, после вызыватся some_action_2 и идти следующий код. Или вы предлагаете, код, который идёт после some_action_1 и some_action_2 в отдельные функции запихнуть?
void after_some_action_1(void) {
// код
}
void after_some_action_2(void) {
// код
}
int func(void) {
int err = some_action_1();
if(err == 0) {
after_some_action_1();
err = some_action_2();
}
if(err == 0) after_some_action_2();
return err;
}Теперь вложенных ифов, конечно, нет. Но код стал сложнее для восприятия. Обосновывается же это только странными правилами стандарта, а не реальными причинами.
Как хорошо, что мне не надо писать код по этому стандарту.
(я подозреваю, что если пришлось бы, я бы нашел тулинг, который берет код, написанный не по стандарту, а потом его преобразует)
ISO26262 для для разработки системного софта для автомобильный ECU для таких автомобилей как BMW, Daimler, Tesla, Ford и проч.
Как хорошо, что мне не надо писать код по этому стандарту.
Ну Вы же сами понимаете, что программировать в стиле каля-маля
это совершенно не дело при разработке ответственных систем.
Поэтому и появились такие стандарты как ISO-26262
Следование подобным стандартам - не единственный способ писать хороший код.
Следование подобным стандартам - не единственный способ писать хороший код.
Хорошо.
И какие же по Вашему существуют ещё способы, стандарты, правила писать хороший код?
Смотря на строчку avoid global variables or justify their usage вспоминается отчёт о коде Тойоты, полученный когда её судили жертвы "внезапного ускорения". Тойота его пыталась списать на застрявшую педаль, но причиной был софт конечно. Мешанина из сотен глобальных переменных, с доступом из разных потоков, меня впечатлила.
Зато все согласно ISO: 1 return и без выделения памяти.
вспоминается отчёт о коде Тойоты, полученный когда её судили жертвы "внезапного ускорения". Тойота его пыталась списать на застрявшую педаль, но причиной был софт конечно.
Вот-вот ISO26262 написан кровью!
Более правильный вывод - ISO26262 и кровь абсолютно никак между собою не связаны. Точнее, хорошо, если там не окажется положительной корреляции. Потому что предполагаю, что наличие искусственных ограничений порождает более громоздкие конструкции, которые сложнее анализировать и верифицировать. А примитивность правил напоминает скорее религию и карго-культ, чем реальную попытку увеличить надежность чего-то там.
Ваш код и пример выше этот тезис прекрасно демонстрируют - нет двойных return, все переменные освящены православным священником, зато есть повреждение памяти и потенциальное удаленное исполнение произвольного кода (кто-то настраивает MPU в своих проектах?), если вдруг вход будет злонамеренным.
И бонусом уже отвлеченный вопрос - а вы разрабатываете софт для автомобилей? Если да - то для каких именно?
Да, думаю корреляция положительная - потому что стандарты вроде ISO26262 в реальности используют не для улучшения качества, а для снятия с себя с ответственности - когда их будут судить, будут отбиваться что мол пользуются лучшими стандартами индустрии. Для улучшения же качества нужны не дурные жесткие стандарты, а реальное понимание разработчиками что они делают, и желание подумать о потенциальных ошибках.
Так и тут - single return помог автору неправильно обработать нехватку буфера out_buff, возвращая true вместо false. Хороший разработчик забил бы на single return, и не сделал бы этой ошибки. Зато стоило бы написать fuzzer test в пять строчек, и тут же нашелся бы buffer overflow.
Исправил, Обновил. Теперь этот тест проходит.
bool test_csv_parse_text_overflow(void){
LOG_INFO(TEST, "%s():", __FUNCTION__);
bool res = true;
set_log_level(CSV, LOG_LEVEL_DEBUG);
char sub_text[5] = "";
memset(sub_text, 0, sizeof(sub_text));
ASSERT_FALSE( csv_parse_text("aa;123456789;cc",';', 1, sub_text, sizeof(sub_text)) );
set_log_level(CSV, LOG_LEVEL_INFO);
return res;
}
Более правильный вывод - ISO26262 и кровь абсолютно никак между собою не связаны
Toyota: 81 514 нарушений в коде
Ужас, если писать код, не приходя в сознание - машины будут врезаться в деревья.
Если у вас разработчики генерят ТАКОЕ - то нужно не правила про двойной return применять, а эвтаназию вводить. Потому я продолжаю утверждать, что ISO26262 и кровь абсолютно никак не связаны.
Не нужно никакого ISO26262, чтобы понять, что код тойоты плохой. Для этого достаточно наличия глаз (чтобы прочитать код с монитора) и головы (для первичной обработки принятого оптического сигнала). А если вы гениям, сотворившим такое, дадите ISO26262 - там появится еще 20000 строчек, чтобы обойти его запреты и все равно написать говно с глобальными переменными и гонками.
Нельзя делать двойной return? Реализуем ту же логику с одинарным, по пути добавив багов. Нельзя делать глобальные переменные вообще? Передадим указатель на локальную структуру в три задачи одновременно и будем туда писать без синхронизации. По пути засунем некоторые переменные в стек, словив его переполнение (разумеется, никто о нём не узнает, про MPU ничего не написано в ISO26262, оно просто тихо испортит данные). Цикломатическая сложность? Побьем лапшу на бессмысленные функции по 10 строк каждая, подумаешь проблема. С правила про вложенные /* вообще поржал.
А на все претензии будем делать глаза трехмесячного щенка и говорить - да у нас же всё по ISO 12345, ISO 54321, ISO 00000, ISO 666 и ISO 999. Попутно такие же аргументы будут заменять реальный аудит и ревью кода - зачем аудит, всё по стандартам индустрии, оно точно безопасное.
Тут даже Rust не факт, что помог бы, хотя вот там реально сложно случайно сделать небезопасную программу. Хотя двойной return там разрешен.
После использования в коде strcpy (за который даже студентов бьют по рукам) довольно странно рассуждать про вред двойного return. Тут вы его проверили, но раз вы уже посчитали длину - можно было использовать memcpy.
P.S. А, там еще помимо strcpy (который корректный, но в целом считается плохой практикой) есть переполнение temp, которое уже нигде не проверяется. Про тихие "return true" при ошибках вам уже сказали.
Норм, код некорректный, небезопасный, нечитаемый, зато без двойного return.
Жесть код.
Копирование в temp не проверяет на переполнение - надеясь на то что любой CSV будет соблюдать вашу магическую константу CSV_VAL_MAX_SIZE.
Само это копирование идет постоянно, csv_cnt_acc_proc даже не смотрит на fetch_index, копирует все значения подряд, видимо чтобы всё замедлить, другого смысла не вижу.
Зачем там вообще этот temp, проще же запомнить адрес начала текущего значения и ничего не копировать, не нужен огромный буфер temp, не нужна магическая константа, не нужно бесполезное копирование всего увиденного текста.
Возвращая значение, размер буфера уже проверяется, но при переполнении всего-навсего делается логгинг, и возвращается
true, как будто всё в порядке.И это копирование ровно так же не нужно, можно просто вернуть string_view или аналог, указывающий на буфер.
Вы понимаете, что нормальный разбор CSV, со строгой поддержкой кавычек и переводов строки, может работать на порядки быстрее написанного, не говоря уже о надежности?
Жесть код.
Предоставьте тест-case, который покажет в run-time, что код из текста не работает.
Переполнение буфера проблемой не считаете? Или в коде есть хоть одна проверка на CSV_VAL_MAX_SIZE?
P.S. обижаться и минусовать карму за детальнейший code review - фи. Первое чему стоит научиться разработчику - это ценить любой code review, а не воспринимать критику кода в штыки.
А чтобы показать работает или нет, нужна спецификация. Дан же прототип функции, а что за bool она возвращает - не сказано. По мне, невозможность различить по результату функции, кроме анализа логов, две ситуации - вернули ли нам значение из CSV, или не хватило переданного буфера и out_buff остался нетронутым - большая проблема, а код в обоих случаях вернет true. Но поскольку спецификация не говорит, что мы возвращаем, то можно сказать всё ок :)
Копирование в temp не проверяет на переполнение - надеясь на то что любой CSV будет соблюдать вашу магическую константу CSV_VAL_MAX_SIZE.
Исправил, код обновил.
Само это копирование идет постоянно, csv_cnt_acc_proc даже не смотрит на fetch_index, копирует все значения подряд, видимо чтобы всё замедлить, другого смысла не вижу.
Зачем там вообще этот temp, проще же запомнить адрес начала текущего значения и ничего не копировать, не нужен огромный буфер temp, не нужна магическая константа, не нужно бесполезное копирование всего увиденного текста.
Возвращая значение, размер буфера уже проверяется, но при переполнении всего-навсего делается логгинг, и возвращается
true, как будто всё в порядке.
Исправил. Обновил текст.
К сожалению, тут реализация заведомо ограничена неудачным контрактом.
На деле же просится, во-первых, полный разбор строки за один проход, во-вторых, попутный анализ потенциального типа каждого элемента. Например, если там не содержится ничего, кроме знаков +/-, десятичного разделителя и цифр - это потенциально число (может быть преобразовано в число). Иначе - строка. Также (при необходимости) можно распознавать потенциальные дату, время.
Если говорить конкретно о NMEA, то встречались простые, быстрые и компактные парсеры на С (NMEAlib, NMEA...) где автоматически распознается тип строки и сразу возвращается заполненная соответствующая структура.
Но даже при таком контракте можно что-то выморщить - если сценарий использования подразумевает множественный вызов для одной строки с извлечением 1-го, 2-го и далее элементов, можно на первом вызове сделать полный разбор с занесением всех элементов во внутренний статический массив, а для всех последующих (если строка та же), сразу возвращать соотв. элемент массива.
Как вариант, не хранить элементы, но только разметку - позиции начала каждого элемента в строке. Тогда на втором и последующем вызовах для данной строки не потребуется заново проходить по всей строке, достаточно просто вытащить из нее кусок от начала нужного элемента до начала следующего за ним.
На деле же просится, во-первых, полный разбор строки за один проход,
Дело в том, что нельзя использовать динамическое выделение памяти. А в CSV строке может быть десятки элементов. Поэтому на заранее не известно какого размера нужен массив, чтобы складировать результат.
в CSV строке может быть десятки элементов.
Количество элементов в протоколе NMEA известно же?
Вообще-то, насколько я понимаю, речь идет не о CSV строках "ва-аще", а о каком-то конкретном случае, когда формат строки в целом известен.
И я очень сомневаюсь, что контроллер, где недоступно динамическое выделение памяти, будет работать с CSV строками в несколько мегабайт и тысячами элементов.
На 99.9% уверен, что есть заранее известный формат CSV строки (будь то NMEA поток или какие-то служебные сообщения) который нужно разбирать. И в условиях жесткого ограничения ресурсов первый путь оптимизации делать не универсальное решение на все случаи жизни, но конкретное решение под конкретную задачу с учетом все е особенностей.
А решение каждый раз елозить по всей строке в поисках нужного элемента... Ну мне, по крайней мере, это кажется лютым костылингом и полным отсутствием сколь-либо продуманной архитектуры решения.
Если там речь пойдет о пресловутых мегабайтных строках с тысячами элементов, то извлечение 763-го элемента в такой реализации будет занимать дико много времени.
И даже в таком случае я бы постарался как-то оптимизироваться. Например, запоминать номер и позицию последнего выбранного элемента в строке и следующий поиск начинать от него - нужен предыдущий - идем назад (или смотрим будет он ближе к последнему или к началу строки), нужен следующий - идем дальше от последнего обработанного. А если там вообще последовательно (1, 2, 3,...) и других сценариев нет - совсем просто. Запомнили что для этой строки в прошлый раз извлекали второй элемент, строка та же, нужен третий - значит идем по строке с того места, где в прошлый раз остановились.
Короче говоря, нужно всегда учитывать как контракт, так и сценарий его использования и от этого строить реализацию.
Автор, похоже, хотел натянуть сову парсинга csv на глобус конечных автоматов ;)
Если нужно парсить данные gps-приемника, формат которых известен заранее - что мешает, раз уж есть strcpy, взять strtok, или сразу scanf?
Автор, похоже, хотел натянуть сову парсинга csv на глобус конечных автоматов ;)
И написать его вручную... Засунуть грамматику в генератор парсеров и получить весь этот конечный автомат автоматически - наверное, адекватнее было бы.
Ладно бы regexp'ы нужно было парсить. Но для выбранной задачи это как пушкой по воробьям, только не стрелять а ездить.
Засунуть грамматику в генератор парсеров и
Что можно почитать про генераторы парсеров? Что это за технология такая?
Вы пишите файл с грамматикой, генератор парсеров на его основе генерирует код для синтаксического разбора языка, определяесого заданной грамматикой. Почитайте про bison, например.
Я старый и немодный, в мое время это были LEXX и YACC (в виндовых версиях FLEX и BISON).
Может сейчас чего помощнее и посовременнее есть.
Тулзы, позволяющие разрабатывать парсер и лексический анализатор. Можно свой скриптовый язык сделать +я делал, даде работало, давно, правда, было...)
А они (BISON и FLEX) могут сделать парсер для бинарного протокола c контрольной суммой (типа ModBus)?
Короткое гугление показывает, что ModBus - это протокол, а не формат (или язык). Для него нет грамматики. Значит, нет и парсера.
В общем, да.
Lex — это инструмент для лексического анализа, который может использоваться для выделения из исходного текста определенных строк заранее заданным способом. Yacc — это инструмент для грамматического разбора; он читает текст и может использоваться для конвертирования последовательности слов в структурированный формат для дальнейшей обработки.
Не скажу, не уверен. Насколько помню, там все строилось на описании грамматики языка.
Может быть как-то можно туда бинарное накрутить, но проще руками (как мне кажется).
Погуглите лексс и якк просто для понимания что там и как
Вам может подойти kaitai struct, там как раз можно описать на декларативном языке бинарный формат, и он сгенерирует вам код, который с этим форматом работает.
Ошибся по старости лет LEX с одной X
Разновидности - FLEX (Fast LEX) и GNU BISON
Но это именно про тексты.
Из относительно современного ANTLR, например.
Очень удобно на самом деле.
ABNF уже есть даже: https://datatracker.ietf.org/doc/html/rfc4180#section-2
(BTW, этот RFC явным образом описывает поведение для полей, содержащих разделители, так что парсер из поста этот RFC не реализует)
CSV это просто последовательность символов, которые разделены запятой или любым другим одиночным символом
Позвольте не согласиться. Как уже отметили другие, в формате CSV есть особые правила для обработки многострочных значений и значений, содержащих спецсимволы (в том числе и сам разделитель), такие значения оборачиваются в кавычки. Наивные парсеры не учитывают такие спец. значения.
Потом, любой URL это, в сущности, та же самая пресловутая CSV строчка
Опять же, не все так просто https://en.wikipedia.org/wiki/Uniform_Resource_Identifier и в общем случае идентификатор ресурса зависит от схемы, имеет в себе компоненты (scheme, userinfo, host, port, path, query, fragment), и разделителями компонент являются разные группы символов.
К чему это я? Если есть возможность использовать готовую отлаженную библиотеку, то лучше использовать ее, нежели изобретать еще один велосипед на квадратных колесах.
Как это обычно и бывает в программировании все задачи решаются золотым шаблоном: конечным автоматом.
Нет в программировании "золотых шаблонов", как и серебрянных пуль.
А конкретно у вас - не синтаксический разбор CSV-строчек (потому что, как вам уже написали, вы не разбираете типовый для CSV случай вложенного разделителя), а решение какой-то частной нужной вам задачи, которую вы потом пытаетесь выдать за общую.
Мне много раз приходилось загружать данные из CSV (но ни разу не пришлось писать парсер), и контракт "Также дан индекс элемента в виде положительного целого числа [...] Вернуть подстроку, которая соответствует индексу" мне не был нужен никогда - если у меня есть CSV, мне почти всегда нужно больше одного значения оттуда, поэтому строка должна парситься один раз, а не столько раз, сколько мне из нее нужно значений.
мне почти всегда нужно больше одного значения оттуда, поэтому строка должна парситься один раз, а не столько раз, сколько мне из нее нужно значений
Ну на самом деле так оно и есть. Но если ресурсы ограничены и нет возможности вывалить весь массив целиком, то почему бы не сделать последовательный парсинг - если передана новая строка, то выделили первый элемент, вернули его, запомнили место где остановились. Если передана та же строка - вернули следующий элемент и опять запомнили точку остановки. Если правильно помню, именно так работает strtok - пока его не "переинициализируют" новой строкой, он возвращает указатель на следующий токен в той строке, с которой работал прошлые вызовы.
Ровно также работают выборки из БД - если объем элемента выборки, скажем, 2кБ, а в выборку попало, скажем 25млн элементов - никто не будет их все сразу в память тянуть. Будет fetch в цикле, который возвращает один (или несколько - блок - элементов) и дальше обработка того, что вернулось. Но никому не придет в голову каждый раз перебирать всю выборку с начала пока не дойдешь до нужного элемента.
Что касается контракта - он может быть неудачен как в силу неправильное изначальной постановки, так и быть наследием предыдущей реализации. И тут надо смотреть где этот контракт используется. Если локально в паре мест, то, возможно, проще поменять контракт с локальными правками и привести его к нормальному виду, соответствующему текущей реализации. Но тут надо глубже в задачу вникать, естественно.
Но если ресурсы ограничены и нет возможности вывалить весь массив целиком, то почему бы не сделать последовательный парсинг - если передана новая строка, то выделили первый элемент, вернули его, запомнили место где остановились.
Это все равно однократный парсинг, о чем я и говорю. Просто последовательный, а не одномоментный.
и контракт "Также дан индекс элемента в виде положительного целого числа [...] Вернуть подстроку, которая соответствует индексу" мне не был нужен никогда
Такой API на самом деле очень удобен для модульных тестов.
Не надо определять техническое задание по потребностям модульных тестов, особенно когда это плохо сказывается на характеристиках результата.
Не говоря о том, что "типовой" контракт ничем не хуже для тестов.
Когда код пишут без намерения его тестировать, то приводит это потом к спагетти коду и вот таким жалким оправданиям
Так в чём проблема протестировать парсер, который на каждый чих не разбирает строку заново?@lair и не говорил, что код не надо тестировать. Да и аргумент слабый. Давайте посмотрим на ваш апи, который легко тестировать, как вы сказали. Ваш код получился очень сложным и с ошибками (как вам показали, возможно переполнение буфера). Получается, что дело не только в апи
Я, вроде бы, не предлагал не тестировать код. Я написал, что контракт надо определять не через тесты, а через реальное использование. Тестировать его после этого все равно можно (и нужно).
Спагетти-код - спагетти-тесты.. Говнокод - говнотесты ;)
Не вижу, каким образом пожелание "чтобы модульные тесты" противоречит пожеланию "решать задачу бизнеса".
Код пишется чтобы решить задачу бизнеса. Тесты это проблема разработки. Любой нормальный код решающий задачу бизнеса можно протестировать. Написание тестов это вообще не критерий для выбора АПИ и контрактов.
Можно написать код который будет решать задачу бизнеса.
При этом там будут функции по 5000 строк, магические циферки на каждой строчке, доступ в регистры в каждом файле, дублирование кода, вставка препроцессором #include *.c файлов и тому подобное.
И такой код невозможно будет покрыть тестами. Потому что он весь написан как одна мега-функцияй main().
Это так разработать электронную плату без тестовых падов.
Вы точно пишете продакшен код для бизнеса? Просто вы такие странные штуки противопоставляете друг-другу что закрадываются сомнения.
Если что это перпендикулярные измерения. Они вообще никак не связаны. Нужно писать просто хороший код для решения задач бизнеса. Есть много мест где именно так и поступают. Методики тестирования уже достаточно развиты чтобы не писать продакшен код для тестирования. Можно тестировать любой нормальный код.
Мне всегда казалось что тесты пишутся под код, но никак не наоборот.
Все контракты все-таки разрабатываются для получения максимальной эффективности для заданного (!!!) функционала, а не "вообще на все случаи жизни" и не "чтобы тестировать проще было".
Мне всегда казалось что тесты пишутся под код, но никак не наоборот.
В TDD - не так. В TDD предполагается, что тесты пишутся под требования (т.е. под предполагаемый сценарий использования), а уже потом код пишется под тесты.
Но я не говорю, что это обязательный подход, просто напоминаю, что так тоже можно, и это работает.
Ну черт его знает... Интуитивно не кажется оптимальным, но не значит что это на самом деле так.
Мне казалось, что решение реализуется для наиболее эффективной реализации сценариев его использования, а тесты - это просто имитация этих сценариев.
Интуитивно не кажется оптимальным, но не значит что это на самом деле так.
У оптимальности много критериев. Для меня для определенных случаев TDD "оптимально", потому что я минимизирую риск написать тесты, которые находятся под влиянием решения, и поэтому не покрывают какой-то edge case. Если думать над тестами до решения, можно (а) найти противоречия в требованиях до написания кода, и (б) написать тесты, которые максимально близки к требованиям (а не к коду).
Но это все субъективно. Мне так удобнее.
В CSV символ-разделитель может встречаться внутри текстового поля. При этом само текстовое поле заключается в кавычки или апострофы. Если в свою очередь внутри текстового поля нужно вставить кавычку (символ-ограничитель) - она, если верно помню, утраивается. Не увидел разбора этой ситуации.
И да, я писал парсер )) (смеркалось… на улице шёл дождь, заняться было нечем, а парсер иметь было бы неплохо…). Он раскладывает многострочный CSV (файл) в QList<QStringList> либо одну строку в QStringList, а дальше можно дёргать по индексам. Кавычки обрабатываются.
Я так понял, что это для какого-то контроллера пишется. А там с ресурсами ну край как туго. Никакое Qt туда не впихнуть просто.
Я так понял, что это для какого-то контроллера пишется. А там с ресурсами ну край как туго. Никакое Qt туда не впихнуть просто.
Да. Именно так.
Да из кутей там только удобный контейнер и работа с файлами, так-то можно и на STL (и даже на голом C)), все равно там посимвольный разбор.
Согласен, для встраиваемого контроллера разбирать и раскладывать в память всю таблицу - невпихуемо, лучше действовать по принципу "достань мне из строки поле № N". Но это не мешает использовать обработку кавычек.
Синтаксический разбор CSV строчек