Many thanks for the review!
sprintf(typelib_id, "#%d", expr->cval);add_output_to_resources("TYPELIB", typelib_id);output_typelib_regscript(typelib->typelib);This is missing an equivalent of 04d8725080fd.
Yes.
We should probably factor out this whole block into write_typelib_regscript().
Probably that could be done as a separate patch.
- put_data(dummy2, sizeof(dummy2));
- put_data(dummy3, sizeof(dummy3));
Do we know what this "dummy" and "pad" data is? If so, can it be named more appropriately?
No, I don't know what these blocks are supposed to contain.
(And if not, why is it separated into groups like this?)
It seems that dummy3 is some magic block, at least dlls/ole32/storage32.c, STORAGE_WriteCompObj() considers it that way. Once its meaning is figured out it will be simpler to find and change similar places in the code.
Same for other places, like sltg_write_nametable().
I couldn't figure out the meaning of the blocks named dummy, otherwise I would name them appropriately.
- /* library block length includes helpstrings and name table */
- entry.length = block->length + 0x40 + 2 + 4 + 6 + 12 + 0x200 + sltg->name_table.size + 12;
This is a lot of magic numbers, and I can't immediately figure out what any of them even are. Is there a better way we can calculate this?
0x40 is the size of pad[0x40] and 0x200 is the name table size. Other numbers also were chosen carefully at the time that code was written. Sorry, I don't remember more details at this point.
(Perhaps by setting this to the value of output_buffer_pos after we write the rest of the block, like you do with name_table_offset below?)
- block = sltg->blocks;
- for (i = 0; i < sltg->n_file_blocks - 1; i++)
Why minus one?
Library block is included in the total number of blocks, however it's written separately.
Moreover, why are we storing the blocks in a hand-rolled linked list, but also keeping a count?
That seemed like a good approach at the time this code was written. It's hard to comment on things created long time ago for a completely not documented file format.
error("add_statement: unhandled statement type %d\n", stmt->type);break;- }
We shouldn't need the default case; we'll get a -Wswitch warning if one is missing.
This chunk was supposed to catch statements created by the .idl parser but not supported by the typelib generator.
- name_table->size = 0;
- name_table->allocated = 0x10;
- name_table->names = xmalloc(0x10);
Is there any point pre-initializing it?
Probably no.