On Tue, Mar 29, 2005 at 10:42:55AM +0200, Karl Pitrich wrote:
> Hi Jody,
>
> we've added property-write support to libgsf and are contributing it
> under LGPL. We're hope that you review the changes and commit them to
> CVS. Please find a patch against last thursday's CVS-HEAD in the
> attached message.
Lovely. I've been hoping someone would finish this off.
> The author of the patch is Manuel Mausz, thus please contact him
> directly for questions regarding the code.
The patch is nice work. I've got some suggestions to improve the
maintainability. Welcome to the fun of open-source and code-review.
> +msole_gsf_name_to_prop_id (char const *name)
> +{
> + GsfMSOleMetaDataPropMap const *map = NULL;
> + unsigned i = 0;
> +
> + map = component_props;
> + i = G_N_ELEMENTS (component_props);
> + while (i-- > 0) {
> + if (!strncmp(map[i].gsf_name, name, strlen(map[i].gsf_name)))
> + return map[i].id;
> + }
> +
> + map = document_props;
> + i = G_N_ELEMENTS (document_props);
> + while (i-- > 0) {
> + if (!strncmp(map[i].gsf_name, name, strlen(map[i].gsf_name)))
> + return map[i].id;
> + }
> +
> + map = common_props;
> + i = G_N_ELEMENTS (common_props);
> + while (i-- > 0) {
> + if (!strncmp(map[i].gsf_name, name, strlen(map[i].gsf_name)))
> + return map[i].id;
> + }
This is wasteful. Please create some hash tables.
> +static GsfMSOleVariantType
> +msole_gsf_name_to_prop_type (char const *name)
and this is duplicate code. Please merge these two routines into
something that does the lookup and returns a
GsfMSOleMetaDataPropMap const *
Then add convenience wrappers if necessary.
> +static guint32
> +gsf_align_to_32bit(gsf_off_t offset) {
I don't see this used in your patch. Why do we need it ?
> +static void
> +add_props(gpointer name, gpointer value, gpointer user_data)
We use a naming convention of
cb_<foo>
for callbacks. It's also preferable to use the right types here in
the decl, and cast the function pointer down below in the foreach.
> +{
> + if (memcmp(name, GSF_META_NAME_LANGUAGE,
> sizeof(GSF_META_NAME_LANGUAGE)) &&
> + memcmp(name, GSF_META_NAME_DICTIONARY,
> sizeof(GSF_META_NAME_DICTIONARY))) {
It seems simpler to map the name to id, then compare those rather
than this.
> +
> + gboolean error = FALSE;
> + guint32 it = ((AddPropsStruct *)user_data)->it;
> + GsfMSOleMetaDataProp_real *props = NULL;
> + props = &((AddPropsStruct *)user_data)->props[it];
See comment re: function argument types above.
> + gsf_off_t offset;
> + GValue *tmp = NULL;
> +
> + GsfDocPropVector *vector = NULL;
> + guint vector_num_values = 1;
> + guint i;
Beware of non-C89 variable declarations, some compilers are not as
permissive as gcc.
> + for (i=0; i < vector_num_values; i++) {
The majority case here will be non-vector. Could we call this
something other than vector_num_values ? That makes this code seem
vector specific.
> + offset += 1;
> + break;
> +
> + case G_TYPE_INT:
> +
> switch(msole_gsf_name_to_prop_type((char *)name)) {
> + case VT_I2:
> + props->type = VT_I2;
> + offset += 2;
> + break;
> +
> + case VT_I4:
> + props->type = VT_I4;
> + offset += 4;
> + break;
To ease readability please merge the VT_I4 and default cases.
> + case G_TYPE_UINT:
> +
> switch(msole_gsf_name_to_prop_type((char *)name)) {
As above.
> + case VT_FILETIME:
> + timestamp = (GsfTimestamp const
> *)g_value_get_boxed((GValue *)prop->value);
> + timet_value = (time_t)timestamp->timet;
> +
> +#ifdef _MSC_VER
> + timet_value += 11644473600i64;
> +#else
> + timet_value += 11644473600ULL;
> +#endif
Can we get this into #define
> +gboolean
> +gsf_msole_metadata_write_real (GsfOutput *out, GsfDocMetaData *meta_data,
> + gboolean doc_not_component, GError **err)
> +{
> + gboolean success = TRUE;
> + gsf_off_t offset = 0;
> + guint8 header[] = {
static guint8 const header[] = {
> + guint32 byte_count = 0; /* count of bytes in a section */
> + guint32 props_count = 0; /* count of propertys global */
> + section_offset += sizeof(byte_count); /* size of section byte count */
> + section_offset += sizeof(props_count); /* size of property count */
I'd rather see these with fixed size of 4 to correspond to the on
disk format rather than tying it to the compiler.
> + if (success) {
> + props_struct = g_new (AddPropsStruct, 1);
> + props_struct->props = props;
> + props_struct->udef_props = udef_props;
> + props_struct->it = 0;
> + props_struct->count = 0;
> + props_struct->dict_count = 0;
> + props_struct->offset = section_offset;
> + props_struct->dict_offset = section_offset;
> + gsf_doc_meta_data_foreach(meta_data, add_props, props_struct);
> + }
Seems like we need to use the 'doc_not_component' kludge here to
filter which of the properties to export.
> + */
> + if (success) {
> + if (prop_codepage != NULL) {
> + // TODO: GSF_META_NAME_LANGUAGE = VT_UI2 - msdn says
> VT_I2
Hmm. The largest values I've seen for this are 10,000 we can't
prove it either way. Let's trust msdn.
> + //GSF_LE_SET_GUINT32 (buf+0,
> msole_gsf_name_to_prop_type(GSF_META_NAME_LANGUAGE);
Watch out for C++ style comments.
> + /*
> + * Write 2. Section
> + */
Why are we duplicating all this code ?
> + if (prop_codepage != NULL)
> + g_free(prop_codepage);
g_free tests for NULL, no need to add wrapper tests.
> + return success;
I worry about situations where things partially fail and the
resulting output is broken. Do you think it's feasible to get a
wrapper to this that would truncate the stream on failure ?
_______________________________________________
gnumeric-list mailing list
[email protected]
http://mail.gnome.org/mailman/listinfo/gnumeric-list
