Table: Clean up column list parsing

First of all, an empty list of columns is a little bit obscure,
but still correct and it should work properly.

table_col_order_by_name() now returns a sensible error message
for non-existent columns.

table_get_col_list() now uses ", " as a separator, which is more
readable. It is documented where the string is allocated from. Also,
it uses the growing string in mempool properly, even in cases when
it is reallocated.

table_start() does not leak memory.

diff --git a/ucw/table-test.t b/ucw/table-test.t
index f2871a4537a757972f233dcf4e7b38a8618d708b..6a0f54a2ca72d026805c3288c2c0c62de436a69e 100644 (file)
--- a/ucw/table-test.t
+++ b/ucw/table-test.t
@@ -67,7 +67,7 @@ EOF
 
 Run: ../obj/ucw/table-test -n
 Out <<EOF
-Tableprinter option parser returned: 'Invalid tableprinter column list: possible column names are col0_str,col1_int,col2_uint,col3_bool,col4_double.'.
+Tableprinter option parser returned: 'Unknown table column 'test_col0_str', possible column names are: col0_str, col1_int, col2_uint, col3_bool, col4_double.'.
 EOF
 
 

diff --git a/ucw/table.c b/ucw/table.c
index 006e44f9f057ecf66225c7e804c731d757368dd3..a21f08aaba4055968001263b51485e00948edf8a 100644 (file)
--- a/ucw/table.c
+++ b/ucw/table.c
@@ -64,16 +64,13 @@ void table_start(struct table *tbl)
   tbl->last_printed_col = -1;
   tbl->row_printing_started = 0;
 
-  // FIXME: Memory leak
-  tbl->col_str_ptrs = mp_alloc_zero(tbl->pool, sizeof(char *) * tbl->column_count);
+  if(!tbl->col_str_ptrs) {
+    tbl->col_str_ptrs = mp_alloc_zero(tbl->pool, sizeof(char *) * tbl->column_count);
+  }
 
   if(tbl->column_order == NULL) table_make_default_column_order(tbl);
 
   if(tbl->formatter->table_start != NULL) tbl->formatter->table_start(tbl);
-  if(tbl->cols_to_output == 0) {
-    // FIXME: Why?
-    die("Table should output at least one column.");
-  }
 
   mp_save(tbl->pool, &tbl->pool_state);
 
@@ -108,13 +105,12 @@ int table_get_col_idx(struct table *tbl, const char *col_name)
 
 const char * table_get_col_list(struct table *tbl)
 {
-  if(tbl->column_count == 0) return NULL;
+  if(tbl->column_count == 0) return "";
 
-  // FIXME: This does not work! The start of the string may be reallocated later.
-  char *tmp = mp_printf(tbl->pool, "%s", tbl->columns[0].name);
+  char *tmp = mp_strdup(tbl->pool, tbl->columns[0].name);
 
   for(int i = 1; i < tbl->column_count; i++) {
-    mp_printf_append(tbl->pool, tmp, ",%s", tbl->columns[i].name);
+    tmp = mp_printf_append(tbl->pool, tmp, ", %s", tbl->columns[i].name);
   }
 
   return tmp;
@@ -124,7 +120,7 @@ const char * table_get_col_list(struct table *tbl)
 void table_col_order(struct table *tbl, int *col_order, int cols_to_output)
 {
   for(int i = 0; i < cols_to_output; i++) {
-    ASSERT_MSG(col_order[i] >= 0 && col_order[i] < tbl->column_count, "Column %d does not exist (column number should be between 0 and %d)", col_order[i], tbl->column_count);
+    ASSERT_MSG(col_order[i] >= 0 && col_order[i] < tbl->column_count, "Column %d does not exist (column number should be between 0 and %d)", col_order[i], tbl->column_count - 1);
   }
 
   tbl->column_order = col_order;
@@ -132,27 +128,26 @@ void table_col_order(struct table *tbl, int *col_order, int cols_to_output)
 }
 
 /**
- * TODO: ERROR! this function deliberately causes memory leak.  the
- * problem is that when table_col_order_by_name is called multiple-times,
- * the mp_save adds all the resulting column orders on the memory pool.
- * The memory leak is small, but it is present.
+ * TODO: This function deliberately leaks memory. When it is called multiple times,
+ * previous column orders still remain allocated in the table's memory pool.
  **/
-int table_col_order_by_name(struct table *tbl, const char *col_order_str)
+const char * table_col_order_by_name(struct table *tbl, const char *col_order_str)
 {
-  int col_order_len = strlen(col_order_str);
+  if(!col_order_str[0]) {
+    tbl->column_order = mp_alloc(tbl->pool, 0);
+    tbl->cols_to_output = 0;
+    return NULL;
+  }
 
   char *tmp_col_order = stk_strdup(col_order_str);
 
   int col_count = 1;
-  for(int i = 0; i < col_order_len; i++) {
+  for(int i = 0; col_order_str[i] != 0; i++) {
     if(col_order_str[i] == ',') {
       col_count++;
     }
   }
 
-  struct mempool_state mp_tmp_state;
-  mp_save(tbl->pool, &mp_tmp_state);
-
   int *col_order_int = mp_alloc_zero(tbl->pool, sizeof(int) * col_count);
   int curr_col_order_int = 0;
   const char *name_start = tmp_col_order;
@@ -163,20 +158,17 @@ int table_col_order_by_name(struct table *tbl, const char *col_order_str)
     }
 
     int idx = table_get_col_idx(tbl, name_start);
-    col_order_int[curr_col_order_int] = idx;
     if(idx == -1) {
-      //ASSERT_MSG(idx != -1, "Table column with name '%s' does not exist.", name_start);
-      mp_restore(tbl->pool, &mp_tmp_state);
-      return -1;
+      return mp_printf(tbl->pool, "Unknown table column '%s'", name_start);
     }
-    curr_col_order_int++;
+    col_order_int[curr_col_order_int++] = idx;
 
     name_start = next;
   }
 
   tbl->column_order = col_order_int;
   tbl->cols_to_output = curr_col_order_int;
-  return 0;
+  return NULL;
 }
 
 /*** Table cells ***/
@@ -360,10 +352,9 @@ const char *table_set_option_value(struct table *tbl, const char *key, const cha
       tbl->print_header = tmp;
       return NULL;
     } else if(strcmp(key, "cols") == 0) {
-      // FIXME: We should not exit/abort on errors caused from command line.
-      if(table_col_order_by_name(tbl, value) != 0) {
-        const char *tmp = table_get_col_list(tbl);
-        return mp_printf(tbl->pool, "Invalid tableprinter column list: possible column names are %s.", tmp);
+      const char *err = table_col_order_by_name(tbl, value);
+      if(err != NULL) {
+        return mp_printf(tbl->pool, "%s, possible column names are: %s.", err, table_get_col_list(tbl));
       }
       return NULL;
     } else if(strcmp(key, "fmt") == 0) {
@@ -417,30 +408,26 @@ const char *table_set_gary_options(struct table *tbl, char **gary_table_opts)
 
 static void table_row_human_readable(struct table *tbl)
 {
-  uint col = tbl->column_order[0];
-  int col_width = tbl->columns[col].width;
-  bprintf(tbl->out, "%*s", col_width, tbl->col_str_ptrs[col]);
-  for(uint i = 1; i < tbl->cols_to_output; i++) {
-    col = tbl->column_order[i];
-    col_width = tbl->columns[col].width;
-    bputs(tbl->out, tbl->col_delimiter);
-    bprintf(tbl->out, "%*s", col_width, tbl->col_str_ptrs[col]);
+  for(uint i = 0; i < tbl->cols_to_output; i++) {
+    int col_idx = tbl->column_order[i];
+    int col_width = tbl->columns[col_idx].width;
+    if(i) {
+      bputs(tbl->out, tbl->col_delimiter);
+    }
+    bprintf(tbl->out, "%*s", col_width, tbl->col_str_ptrs[col_idx]);
   }
-
   bputc(tbl->out, '\n');
 }
 
 static void table_write_header(struct table *tbl)
 {
-  uint col_idx = tbl->column_order[0];
-  bprintf(tbl->out, "%*s", tbl->columns[col_idx].width, tbl->columns[col_idx].name);
-
-  for(uint i = 1; i < tbl->cols_to_output; i++) {
-    col_idx = tbl->column_order[i];
-    bputs(tbl->out, tbl->col_delimiter);
+  for(uint i = 0; i < tbl->cols_to_output; i++) {
+    int col_idx = tbl->column_order[i];
+    if(i) {
+      bputs(tbl->out, tbl->col_delimiter);
+    }
     bprintf(tbl->out, "%*s", tbl->columns[col_idx].width, tbl->columns[col_idx].name);
   }
-
   bputc(tbl->out, '\n');
 }
 
@@ -468,14 +455,13 @@ struct table_formatter table_fmt_human_readable = {
 
 static void table_row_machine_readable(struct table *tbl)
 {
-  uint col = tbl->column_order[0];
-  bputs(tbl->out, tbl->col_str_ptrs[col]);
-  for(uint i = 1; i < tbl->cols_to_output; i++) {
-    col = tbl->column_order[i];
-    bputs(tbl->out, tbl->col_delimiter);
-    bputs(tbl->out, tbl->col_str_ptrs[col]);
+  for(uint i = 0; i < tbl->cols_to_output; i++) {
+    int col_idx = tbl->column_order[i];
+    if(i) {
+      bputs(tbl->out, tbl->col_delimiter);
+    }
+    bputs(tbl->out, tbl->col_str_ptrs[col_idx]);
   }
-
   bputc(tbl->out, '\n');
 }
 

diff --git a/ucw/table.h b/ucw/table.h
index ad2173d4a9e965d99b9feedcb7120492bd958ef9..62340fe6572619debbcc431a8954ebbce9ab7eea 100644 (file)
--- a/ucw/table.h
+++ b/ucw/table.h
@@ -198,11 +198,9 @@ void table_col_order(struct table *tbl, int *col_order, int col_order_size);
 
 /**
  * Sets the order in which the columns are printed. The specification is a string with comma-separated column
- * names.
- *
- * FIXME: What does the return value mean?
+ * names. Returns NULL for success and an error message otherwise.
  **/
-int table_col_order_by_name(struct table *tbl, const char *col_order);
+const char * table_col_order_by_name(struct table *tbl, const char *col_order);
 
 /**
  * Called when all the cells have filled values. The function the prints a table row into the output stream.
@@ -228,9 +226,8 @@ void table_append_printf(struct table *tbl, const char *fmt, ...) FORMAT_CHECK(p
 int table_get_col_idx(struct table *tbl, const char *col_name);
 
 /**
- * Returns comma-separated list of column names.
- *
- * FIXME: Allocated from?
+ * Returns comma-and-space-separated list of column names, allocated from table's internal
+ * memory pool.
  **/
 const char * table_get_col_list(struct table *tbl);