Thanks for the comments! I’ll submit an updated patch in a day or two. With regards to the oflag, I just haven’t implemented it yet. I’ll try to include it in the next patch.
> On Feb 16, 2015, at 5:27 AM, Dimitris Papastamos <sin_AT_2f30.org> wrote:
>
> Some inline comments below:
>
> +/* See LICENSE file for copyright and license details. */
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <ctype.h>
>
> Sort these.
>
> +static int tflag = 0;
> +static int aflag = 0;
> +static int eflag = 0;
> +/*static int oflag = 0;*/
> +static int oneflag = 0;
> +static int twoflag = 0;
>
> oflag is unused?
>
> +static char *estr = "";
> +static char *empty_str = "";
> +static int join_index1 = 1;
> +static int join_index2 = 1;
> +static int delim = ' ';
> +static int (*is_delim)(int);
> +static int afile;
> +static int print_unjoinable1 = 0;
> +static int print_unjoinable2 = 0;
>
> I'd prefer shorter names without underscores.
>
> +struct line_list_t {
> + int cur_lines;
> + int capacity;
> + struct line_t *lines;
> + struct line_t *next;
> +};
>
> Use queue.h, do not roll your own lists. Do not
> add a _t suffix to the struct name. You can use a typedef
> here as explained in the coding conventions[0]. Use
> less verbose names, like "cap" or "cur".
>
> +struct line_t {
> + char *fields;
> + char *join_field;
> + int num_fields;
> + size_t len;
> + size_t buf_size;
> + int f_idx;
> + int iter_capacity;
> + char **iter_fields;
> + struct line_t *link;
> +};
>
> Ditto.
>
> +static void
> +free_line_list(struct line_list_t *list)
> +{
> + struct line_t *tofree = 0;
> + struct line_t *line = list->lines;
> +
> + if (list->next && list->next != list->lines) {
> + free(list->next->fields);
> + free(list->next);
> + }
> +
> + while (line) {
> + tofree = line;
> + line = line->link;
> + free(tofree->fields);
> + free(tofree->iter_fields);
> + free(tofree);
> + }
> +}
>
> Remove this function, we can rely on the OS to do this. This is only
> called at the end once so it doesn't really save us anything.
>
> +void
> +resize_iter(struct line_t *line) {
>
> "{" should be on the next line on its own.
>
> + size_t new_size;
> +
> + line->iter_capacity = line->iter_capacity * 2;
> + new_size = line->iter_capacity * sizeof(*line->iter_fields);
> + line->iter_fields = realloc(line->iter_fields, new_size);
> + if (!line->iter_fields)
> + enprintf(2, "malloc: ");
> +}
>
> Use libutil/ as much as possible. Do not use realloc, use erealloc.
> The error message is wrong too.
>
> +struct line_t *
> +create_line(void)
> +{
> + struct line_t *new = calloc(1, sizeof(struct line_t));
> + if (!new)
> + enprintf(2, "malloc: ");
> + new->iter_capacity = 10;
> + new->iter_fields = calloc(10, sizeof(*new->iter_fields));
> + if (!new->iter_fields)
> + enprintf(2, "malloc: ");
> + return new;
> +}
>
> Ditto.
>
> +static int
> +get_line(FILE *fp, struct line_t *line, int ji)
> +{
> + line->len = getline(&line->fields, &line->buf_size, fp);
> + if (-1 == line->len)
>
> Never use this style. I understand the reasoning but it is extremely
> non-idiomatic. The correct condition is:
>
> if (line->len == -1)
>
> I think you should also have a look at libutf/. We want the tools in
> sbase to be UTF-8 aware.
>
> [0] http://suckless.org/style
>
> Cheers,
> Dimitris
>
Received on Mon Feb 16 2015 - 18:45:34 CET