Re: winedump: Add initial version of old (SLTG) typelib dumper.

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> +static int _isprint(unsigned char c)
>  {
> - int i;
> - return c >= 32 ? 1 : 0;

Please don't do that.
> -
> - assert(print_hex("magic") == 0x47544c53);

Please don't do that.
> - while (offset < name_table_start + name_table_size)
> - {
>
> +/*printf("current offset: %08x (relative to name table start: %08x (%u))\n",
> -
>    offset, offset - name_table_start, offset - name_table_start);*/
> -
>    dump_binary(8);
> -
>    print_string0();
> -
>    printf("\n");
>
> +/*printf("current offset: %08x (relative to name table start: %08x (%u))\n",
> -
>    offset, offset - name_table_start, offset - name_table_start);*/

Please clean these up.
> +#if 0
> - print_short_hex("unknown");
> - print_short_hex("byte order mark");
> - i = tlb_read_short();
> - printf("version = %u.%u\n", i & 0xff, i >> 8);
> - print_short_hex("system identifier");
> - print_hex("unknown");
> - printf("\n");
>
> +#else
> - printf("skipping 12 bytes\n");
> - dump_binary(12);
> - printf("\n");
>
> +#endif

Same here.

winedump: Add initial version of old (SLTG) typelib dumper.

[Dmitry Timoshkov]

Alexandre Julliard  wrote:
> > +static int _isprint(unsigned char c)
> >  {
> > - int i;
> > - return c >= 32 ? 1 : 0;
>
> Please don't do that.

Could you please be more specific?
> > -
> > - assert(print_hex("magic") == 0x47544c53);
>
> Please don't do that.

Once again please be more verbose, I can't read your mind. But as a guess
you didn't like the assert()? What's wrong with this one in comparison to
many others in this file?
> > +#if 0
> > - print_short_hex("unknown");
> > - print_short_hex("byte order mark");
> > - i = tlb_read_short();
> > - printf("version = %u.%u\n", i & 0xff, i >> 8);
> > - print_short_hex("system identifier");
> > - print_hex("unknown");
> > - printf("\n");
> >
> > +#else
> > - printf("skipping 12 bytes\n");
> > - dump_binary(12);
> > - printf("\n");
> >
> > +#endif
>
> Same here.

Since this is a work in progress the intent was to show that the format of
this block is unknown and there is at least another source of information
that treats it differently. I'd prefer to leave this as a self documenting
explanation.

winedump: Add initial version of old (SLTG) typelib dumper.

[Alexandre Julliard]

Dmitry Timoshkov  writes:
> Alexandre Julliard  wrote:
> > > +static int _isprint(unsigned char c)
> > >  {
> > > - int i;
> > > - return c >= 32 ? 1 : 0;
> >
> > Please don't do that.
>
> Could you please be more specific?

The ? 1 : 0 is just noise.
> > > - assert(print_hex("magic") == 0x47544c53);
> >
> > Please don't do that.
>
> Once again please be more verbose, I can't read your mind. But as a guess
> you didn't like the assert()? What's wrong with this one in comparison to
> many others in this file?

There's a fundamental difference, think about it for a while (hint:
consider what happens with -DNDEBUG).
> > > +#if 0
> > > - print_short_hex("unknown");
> > > - print_short_hex("byte order mark");
> > > - i = tlb_read_short();
> > > - printf("version = %u.%u\n", i & 0xff, i >> 8);
> > > - print_short_hex("system identifier");
> > > - print_hex("unknown");
> > > - printf("\n");
> > >
> > > +#else
> > > - printf("skipping 12 bytes\n");
> > > - dump_binary(12);
> > > - printf("\n");
> > >
> > > +#endif
> >
> > Same here.
>
> Since this is a work in progress the intent was to show that the format of
> this block is unknown and there is at least another source of information
> that treats it differently. I'd prefer to leave this as a self documenting
> explanation.

A #if 0 doesn't really explain anything. If you are not sure about the
format, a FIXME comment instead would be better.