2017-06-06 14:28 GMT+02:00 Paul Gofman gofmanp@gmail.com:
+static void pres_float_to_int(void *in, void *out, unsigned int count)
"in" should probably be const in all of these helpers.
+static void pres_bool_to_int(void *in, void * out, unsigned int count)
Stray whitespace after *.
+{
- unsigned int i;
- const float *in_bool = in;
- int *out_int = out;
- for (i = 0; i < count; ++i)
out_int[i] = !!in_bool[i];
Is the double negation necessary here (or above)?
+static void regstore_set_data(struct d3dx_regstore *rs, unsigned int table,
unsigned int offset, unsigned int *in, unsigned int count, enum pres_value_type param_type)+{
- typedef void (*conv_func)(void *in, void *out, unsigned int count);
- static conv_func set_const_funcs[PRES_VT_COUNT][PRES_VT_COUNT] =
Missing const.
- {
{NULL, NULL, pres_float_to_int, pres_value_to_bool},{NULL, NULL, NULL, NULL},{pres_int_to_float, NULL, NULL, pres_value_to_bool},{pres_bool_to_float, NULL, pres_bool_to_int, NULL}- };
- enum pres_value_type table_type = table_info[table].type;
- if (table_type == param_type)
- {
regstore_set_values(rs, table, in, offset, count);return;- }
IIUC in this case regstore_set_values() will have in == out. That's a problem since it means memcpy()ing the data onto itself, which is undefined. It's also unnecessary, regstore_set_modified() would be enough, but only if in == out which is something the current regstore_set_data() interface doesn't enforce (and indeed patch 2/5 doesn't respect).
I guess the root issue is that you're now using the regstore also as temporary storage and none of the existing (and new) code expects it. So it's a matter of fixing the code as necessary to make it work fine or to avoid this "new" usage of the regstore e.g. by means of a separate temporary buffer.
for (element = 0; element < const_set->element_count; ++element) {
unsigned int *out = (unsigned int *)rs->tables[table] + start_offset;
Aside from the issue I mentioned above, this works only because all the formats we're interesting in converting from / to happen be 4 bytes wide. It's somewhat assumed in a bunch of places already but adding more isn't too exciting. I'd like at least an assert checking for the precondition, if possible.
@@ -1083,23 +1155,15 @@ static void set_constants(struct d3dx_regstore *rs, struct d3dx_const_tab *const WARN("Parameter data is too short, name %s, component %u.\n", debugstr_a(param->name), i); break; }
in = (unsigned int *)data + param_offset;switch (table_type){case PRES_VT_FLOAT: set_number(&out, D3DXPT_FLOAT, in, param->type); break;case PRES_VT_INT: set_number(&out, D3DXPT_INT, in, param->type); break;case PRES_VT_BOOL: set_number(&out, D3DXPT_BOOL, in, param->type); break;default:FIXME("Unexpected type %#x.\n", table_info[table].type);break;}regstore_set_values(rs, table, &out, offset, 1);
out[offset] = data[param_offset];
I think separating the matrix transposing / "reshaping" from the data conversion is okay, if that allows further changes and optimizations like patch 2/5. You might want to add a small comment around here to clarify the split.
} } start_offset += get_offset_reg(table, const_set->register_count); data += param->rows * param->columns; }
start_offset = get_offset_reg(table, const_set->register_index);regstore_set_data(rs, table, start_offset, (unsigned int *)rs->tables[table] + start_offset,get_offset_reg(table, const_set->register_count) * const_set->element_count, param_type);
I understand that you want to avoid to use some separate storage for the "to be converted" data but, like above, this regstore table pointer cast makes me a bit uncomfortable.