Sorry for the belated review, the patch is big and took me some time to
read and digest.
My comments:
1) elapi_basic.h:40
#define SEDO struct elapi_data_out
I personally don't like this, this seems like black magic. Why not just
use the struct name? I know it's a little long to type, but that's what
code completion is for.
Yes. We had a lengthy discussion about this. I opened a ticket #166 to
address it.
2) elapi_grow_data()
I think it makes sense to rename the "tmp" variable to something like
"newbuf" and change its type to "char *" since the
out_data->buffer is a
char array anyway.
Ok will do.
3) file_csv_data_len
I do not like the magic constants like "20" and such in the switch. I
would prefer #defining them as named constants.
Ok will do.
Martin also had this cool idea of using something like
"strlen(#UINT_MAX)" during configure to get precise values.
Can you please explain how I can use it in configure?
Looks cool but not sure how to do it in configure.
4) struct file_csv_cfg
Is there a reason for defining the qualifier, separator etc. explicitly
as "unsigned char"? Since the buffer is "plain char" which can be
either
signed or unsigned depending on implementation, you can compare
different types, no? I think just explicitly casting to unsigned char
would help.
Can you please elaborate? Are you saying that the buffer should be
unsigned char
or I am misreading your statement?
5) file_print_fmt_cfg() and file_read_fmt_cfg()
I generally prefer {constant, function} tuples over big switches, but
that's just a matter of style I guess
I agree but it is hard to do it in the
first pass.
I opened a ticket (#169) to refactor the code when all the cases are
implemented.
6) file_csv_data_len() and file_serialize_csv()
There seems to be an off-by-one error, for bools, the size is 3 (I
assume this is for string "yes"), but I think it should be 4 to account
the trailing '\0'. Also for bool values, I think "true/false" is more
generally understood than "yes/no".
Jakub, can you please take a look at this function again?
Was I smoking something? Looking at it again it seems that I am
allocating shorter buffers when there is a qualifier for numeric types
and doubles.
I definitely need to redo this function.
9) Nitpicking: using "len" and "length" variables
in the same function
seems confusing
Ok I will come with something better
10) file_get_csv_cfg()
Rather than using magic constants like cfg->csvseparator = ','; I think
it's better to define them into something like DFL_CSV_SEPARATOR
Ok
11) file_build_set()
Maybe it would be less error prone to convert the mode_leftovers to int
using something like strtol instead of doing it manually?
Do not see a big
advantage in this case. I considered strtol but chose
not to use it.
12) Nitpick: bad indenting on file_provider:314 makes the logic harder
to read
Sure will fix.
13) file_read_cfg()
This seems ugly to me:
file_cfg->filename = malloc(strlen(appname) + sizeof(FILE_SUFFIX));
strcpy(file_cfg->filename, appname);
strcat(file_cfg->filename, FILE_SUFFIX);
It left me wondering if it's a off-by-one error until I realized
sizeof(FILE_SUFFIX) accounts the terminating null byte.
I think it's way better to use:
asprintf(&file_cfg->filename, "%s%s", appname, FILE_SUFFIX);
But asprintf it's a GNU extenstion, not sure if it matters for you.
We do not use asprintf() for compatibility reasons.
14) General comment - maybe instead of freeing individual components
after an operation failed, it is less error prone to "goto fail" and
free everything on one place? (I noticed this in file_split_by_set())
(Deep deep sigh) I started programming in C in 1989.
Since then all my C programs were "goto free".
I prefer to keep it this way.
I will send an additional patch with fixes later today.
Thanks for review.
Dmitri