Я реализовал парсер файлов формата ключ: значение в C или что-то похожее на него, поскольку в C. нет такой вещи, как динамическое создание структур.
Предположения: мы создаем программу на C, для которой требуются некоторые параметры конфигурации (например, число K
потоков на параллельном сервере, который обрабатывает запросы, используя пул K
рабочие потоки), и мы хотим, чтобы они читались из текстового файла. Файл содержит пару ключ: значение в каждой строке. В нашей программе мы заранее знаем название параметров и их тип.
fileparser.h
#ifndef FILE_PARSER_H
#define FILE_PARSER_H
typedef struct _parser Parser;
Parser* parseFile(char* filename, char* delim);
void getValueFor(Parser* p, char* key, char* dest, char* defaultVal);
int testError(Parser* p);
void destroyParser(Parser* p);
#endif
fileparser.c
#include "fileparser.h"
#include <string.h>
#include <stdio.h>
#include <stdlib.h>
#include "../scerrhand.h"
#define MAX_FILENAME_LEN 100
#define MAX_DELIM_LEN 1
#define MAX_KEY_LEN 50
#define MAX_VAL_LEN 200
struct _pair {
char key[MAX_KEY_LEN + 1];
char val[MAX_VAL_LEN + 1];
struct _pair* nextPtr;
};
typedef struct _parser {
char filename[MAX_FILENAME_LEN + 1];
char delim[MAX_DELIM_LEN + 1];
struct _pair* hPtr;
int err;
} Parser;
static int cmpKeys(char* k1, char* k2) {
return strcmp(k1, k2);
}
static void _push(struct _pair** hPtr, struct _pair* newPair) {
newPair->nextPtr = *hPtr;
*hPtr = newPair;
}
static int _parse(Parser* p, FILE* fp) {
char buf[BUFSIZ + 1];
struct _pair* kvPair;
size_t i = 0;
while (i++, fgets(buf, BUFSIZ, fp)) {
char* savePtr, * token;
kvPair = calloc(sizeof(struct _pair), 1);
if (!kvPair) {
return -1; // out of memory
}
// get key
token = strtok_r(buf, p->delim, &savePtr);
strncpy(kvPair->key, token, strlen(token));
// remove any trailing spaces after the key name
kvPair->key[strcspn(kvPair->key, " ")] = ' ';
// get value
token = strtok_r(NULL, p->delim, &savePtr);
if (!token) { // delim missing: invalid syntax
return i;
}
strncpy(kvPair->val, token, strlen(token));
// remove trailing newline
kvPair->val[strcspn(kvPair->val, "n")] = ' ';
_push(&p->hPtr, kvPair);
}
return 0;
}
Parser* parseFile(char* filename, char* delim) {
FILE* fp;
Parser* p = calloc(sizeof(Parser), 1);
if (!p) {
return NULL;
}
strncpy(p->filename, filename, MAX_FILENAME_LEN);
strncpy(p->delim, delim, MAX_DELIM_LEN);
SYSCALL_OR_DIE_NULL(fp = fopen(filename, "r"));
// parse file and capture error code
p->err = _parse(p, fp);
fclose(fp);
return p;
}
int testError(Parser* p) {
return p->err;
}
void getValueFor(Parser* p, char* key, char* dest, char* defaultVal) {
struct _pair*
currPtr = p->hPtr,
* prevPtr = NULL,
** target = NULL; // address of the node to remove after consuming its value
while (currPtr && cmpKeys(key, currPtr->key)) {
prevPtr = currPtr;
currPtr = currPtr->nextPtr;
}
// copy value if key is found; otherwise copy default value
strncpy(dest, currPtr ? currPtr->val : defaultVal, MAX_VAL_LEN);
// pop node out of list
if (currPtr) {
target = prevPtr ? &prevPtr->nextPtr : &p->hPtr;
*target = currPtr->nextPtr;
free(currPtr);
}
}
void destroyParser(Parser* p) {
struct _pair* tmp;
while (p->hPtr) {
tmp = p->hPtr;
p->hPtr = p->hPtr->nextPtr;
free(tmp);
}
free(p);
p = NULL;
}
scerrhand.h
#ifndef SC_ERR_HAND_H
#define SC_ERR_HAND_H
#include <stdlib.h>
// makes a system call and exits if return value is not zero
#define SYSCALL_OR_DIE_NZ(s) if(s) { puts("System call failed with nonzero status"); exit(EXIT_FAILURE); }
// makes a system call and exits if return value is -1
#define SYSCALL_OR_DIE_NEG_ONE(s) if((s) == -1) { puts("System call failed with status -1"); exit(EXIT_FAILURE); }
// makes a system call and exits if return value is NULL
#define SYSCALL_OR_DIE_NULL(s) if((s) == NULL) { puts("System call returned NULL"); exit(EXIT_FAILURE); }
#endif
Я хотел бы получить отзывы о дизайне, реализации и многом другом.
2 ответа
Общие наблюдения
Было бы намного проще просмотреть этот код, если бы существовал модульный тест или пример кода, чтобы увидеть, как этот код следует использовать.
Также непонятно, почему код удаляет key value pair
внутри функции getValueFor()
. Имя функции не указывает на то, что она удаляет пару, а также получает значение ключа.
Отчет об ошибках
Есть несколько проблем с SYSCALL_OR_DIE_*
макросы. Во-первых, вы скрываете звонки на exit()
, что может плохо сказаться на техническом обслуживании.
Во-вторых, сообщения об ошибках не очень значимы для человека, запускающего программу, например, я бы предпочел увидеть что-то вроде Can't open file ... for input
скорее, чем System call returned NULL
. Один я знаю, как исправить, как пользователь, другой — я понятия не имею, что это значит. Если бы этот код находился в рабочем состоянии и пользователь получал эти сообщения об ошибках, ему потребовалась бы поддержка, чтобы узнать, как исправить причину проблемы.
Третья проблема заключается в том, что сообщения об ошибках будут stdout
скорее, чем stderr
куда должны идти сообщения об ошибках такого рода. Если вывод программы перенаправляется в файл, то пользователь не узнает о сбое программы до тех пор, пока файл не будет исследован.
Сохранение распределения памяти
Довольно распространенное использование calloc()
использовать то, на что указывает указатель в sizeof()
вызов, потому что это уменьшает объем редактирования, необходимого, если изменение необходимо, например, в следующем коде из static int _parse(Parser* p, FILE* fp)
При изменении типа структуры необходимо изменить 2 строки:
struct _pair* kvPair; // change needed here
size_t i = 0;
while (i++, fgets(buf, BUFSIZ, fp)) {
char* savePtr, * token;
kvPair = calloc(sizeof(struct _pair), 1); // change needed here
if (!kvPair) {
return -1; // out of memory
}
Следующий код легче поддерживать:
struct _pair* kvPair; // Only this line changes
size_t i = 0;
while (i++, fgets(buf, BUFSIZ, fp)) {
char* savePtr, * token;
kvPair = calloc(sizeof(*kvPair), 1); // No need to change this code
if (!kvPair) {
return -1; // out of memory
}
Ненужная упаковка вызовов функций
Непонятно, почему была добавлена или использована следующая функция, поскольку она не меняет возвращаемое значение и не улучшает читаемость:
static int cmpKeys(char* k1, char* k2) {
return strcmp(k1, k2);
}
Если функция возвращала логическое значение, а не int, это могло бы иметь смысл, но просто возвращало значение strcmp()
не добавляет ценности.
Хороший компилятор может просто встроить эту функцию, иначе код добавляет стоимость вызова дополнительной функции.
Именование
C резервирует идентификаторы, начинающиеся с _
за которым следует любая буква для реализации, поэтому они не должны определяться пользовательскими программами.
Я не могу вспомнить, был ли ведущий _
разрешено в struct
теги (это другое пространство имен для имен объектов и функций), потому что проще просто избежать использования ведущих _
где угодно, и никогда не нужно знать точные детали того, что разрешено.
Выход
Я не сторонник выхода из всей программы из служебной функции (особенно если мы создаем библиотеку — это действительно может сбить с ног вызывающего). Мы должны включить <stdio.h>
из scerrhand.h
так что заголовок самодостаточен.
Сообщения об ошибках должны отправляться на stderr
, нет stdout
. Учитывая, что мы используем здесь макросы, у нас есть возможность быть более информативными о том, что именно не удалось (возможно, только в отладочных сборках), используя #s
в заменах. Например:
#define SYSCALL_OR_DIE_NZ(s)
if (s) {
fprintf(stderr, "%s failed with nonzero status", #s);
exit(EXIT_FAILURE);
}
Мы действительно хотим обернуть их в do { … } while (0)
идиомы, чтобы их можно было безопасно использовать в составных операторах (например, if (c) SYSCALL_OR_DIE(something); else SYSCALL_OR_DIE(something_else);
).
Тестирование
Очень жаль, что вы не показали нам юнит-тесты для этого класса. Я думаю, что есть несколько случаев, которые отсутствуют в тестах, как мы вскоре увидим.
Бессмысленная функция
Кажется, что эта функция не имеет значения:
static int cmpKeys(char* k1, char* k2) { return strcmp(k1, k2); }
Мы должны изменить вызывающего абонента для вызова strcmp()
напрямую.
Функция синтаксического анализа
Этот контур управления:
size_t i = 0; while (i++, fgets(buf, BUFSIZ, fp)) {
имеет инициализацию, тест и шаг, поэтому он читается более естественно как for
петля:
for (size_t i = 1; fgets(buf, BUFSIZ, fp); ++i) {
Я не уверен, почему мы используем size_t
для i
но верните его как int
— было бы лучше, если бы эти два типа согласились.
В этом коде мы молча разбиваем все входные строки, длина которых превышает BUFSIZ. Я думаю, что его следует сделать более надежным, с проведением тестов.
Кроме того, почему мы создали buf
с участием BUFSIZE+1
элементы, и никогда не использовать последний? (fgets()
напишу до BUFSIZE
символы, включая завершающий нуль).
kvPair = calloc(sizeof(struct _pair), 1);
Я не понимаю, почему этой переменной нужна вся функция в качестве области действия. Мы можем объявить и инициализировать вместе для более безопасного кода:
struct _pair *kvPair = calloc(sizeof *kvPair, 1);
Неужели нам нужно обнулить память? Думаю, что нет, так как мы сразу записываем туда достоверные данные. Так что используйте простой malloc()
вместо.
Мы полностью упустили суть strncpy()
здесь:
strncpy(kvPair->key, token, strlen(token));
Это просто менее эффективный способ написать strcpy(kvPair->key, token)
, за исключением того, что не записывает нулевой терминатор. Дело в length
аргумент — сообщить функции сколько можно смело писать, но мы утверждаем, что kvPair->key
имеет достаточно места для токена независимо от его длины. В этом случае мы хотим
strncpy(kvPair->key, token, sizeof kvPair->key);
kvPair->key[sizeof kvPair->key - 1] = ' ';
Мы могли бы обойтись улучшенными тестами ключей чрезмерной длины и подумать, можем ли мы добиться большего, чем их усечение.
strcspn()
это излишество, когда мы ищем только одного персонажа — используйте strchr()
вместо.
С использованием strtok_r()
чтобы получить значение, обрежет любое значение, которое может содержать строку разделителя (опять же, добавьте тесты для этого, а также для значений длиннее, чем MAX_VAL_LEN
).
Ранний возврат при пустом значении пропускается free(kvPair)
, что является утечкой памяти. Время от времени выполняйте модульные тесты под Valgrind.
Получатель значения
Меня удивило, что получение ценности — это разрушительный операция. Я также не понимаю, почему key
а также defaultVal
аргументы должны быть изменяемыми — я ожидал const char*
для тех.
Учитывая предлагаемые варианты использования, было бы полезно иметь версии, которые напрямую получают числовые значения (например, как int
, unsigned
и / или double
), а не заставлять пользователя работать со строками.
Файловый интерфейс
Проблемы в parseFile()
в основном похожи на _parse()
.
Деструктор
Объем tmp
может быть сокращен до внутри цикла.
Поручение p
в конце мертвая запись, поскольку сразу после этого переменная выходит за пределы области видимости.
На самом деле нет необходимости постоянно переназначать p->hPtr
внутри цикла — мы можем просто использовать локальный:
void destroyParser(Parser *parser)
{
if (!parser) {
return;
}
struct _pair *p = parser->hPtr;
while (p) {
struct _pair *tmp = p;
p = p->nextPtr;
free(tmp);
}
free(parser);
}
Спасибо! Я не знал о подчеркивании. В соответствии с упаковкой системные вызовы в моем макросе (ах) внутри
do...while(0)
будет ли такой же эффект, если их заключить в фигурные скобки?— Сэмюэл Б.
В
do { … } while (0)
идиома более удобна для пользователя макроса, чем просто фигурные скобки, потому что она может сопровождаться точкой с запятой в контекстах, где нельзя использовать пустой оператор ..— Тоби Спейт
Определенно полезно. Несколько комментариев: что касается строк, размер которых превышает
BUFSIZ
, как бы вы с этим справились? Как вы думаете, я мог бы проверить длину строки с помощьюstrlen
и выдать какую-то ошибку / перераспределить буфер большего размера? Для использованияcalloc
: Я изначально использовалmalloc
, но я получалConditional jump depends on uninitialized memory
от valgrind, который, по-видимому, исчез только тогда, когда я начал использоватьcalloc
вместо.— Сэмюэл Б.
Первым делом нужно хотя бы сообщить о проблеме вызывающему абоненту. Вам нужно будет использовать возвращаемое значение из
fgets()
и посмотрим, прочитал ли он новую строку — в противном случае, строка была слишком длинной для буфера, и мы должны прочитать и сбросить до следующей новой строки и вернуть подходящий код ошибки. После этого я бы разработал код, который может адаптировать и хранить более длинные ключи и значения.— Тоби Спейт
- 1
Проблема, о которой сообщил Valgrind (рад узнать, что вы хорошо используете инструменты), заключается в том, что вы сказали
strncpy
написать только первыйstrlen()
символы строк — в частности, не копирование последнего нулевого символа. Как только вы это исправите, вы будете очищены от Valgrind безcalloc()
.— Тоби Спейт
Спасибо. Я скоро включу модульный тест. @ извлечение элементов из списка после прочтения значения: я согласен с тем, что имя функции не проясняет это. Мое объяснение заключалось в том, что знание того, что список становится меньше по мере того, как вы потребляете значения, сделало бы использование списка, а не, скажем, хеш-таблицы более приемлемым с точки зрения временной сложности. Оглядываясь назад, это довольно глупо. @errors: вы думаете, что макрос принимает строку и печатает ее с помощью
perror
будет работать лучше?— Сэмюэл Б.
@ упаковка функции сравнения: я сделал это на тот случай, если я хочу поддерживать функции сравнения, поставляемые извне, поэтому мне не пришлось бы возвращаться к значению, которое его вызывает, и редактировать его там. Я согласен, это довольно бесполезно, как сейчас
— Сэмюэл Б.