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

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> 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.

-- 
Alexandre Julliard
julliard(a)winehq.org

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

[Dmitry Timoshkov]

Alexandre Julliard <julliard(a)winehq.org> 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.

-- 
Dmitry.

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

[Alexandre Julliard]

Dmitry Timoshkov <dmitry(a)baikal.ru> writes:
> Alexandre Julliard <julliard(a)winehq.org> 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.

-- 
Alexandre Julliard
julliard(a)winehq.org