[Bug-ddrescue] Re: ddrescue patch for unsigned/signed compiler warnings

[Martin Koeppe]

Hi Antonio,

> > Why? As I could see, one block is filled by one readblock(),
> 
> A small block, split from a larger block, is filled by a call to 
> readblock. The result is then joined to the coreresponding larger block. 
> Both larger blocks can have sizes larger than 32 bits. This is why the 
> ddrescue logfile is so efficient.

thank you for the explanation.

> > However, I saw a use of -1 in the size_ field.
> 
> -1 means undefined size. It was used for devices of unknown size.
> 
> 
> > Where exactly does it break? I can make a new version, if you like.
> 
> For example where it moves "#define _FILE_OFFSET_BITS 64" to "os.h" and 
> then includes "os.h" after cstdio. This makes ddrescue unable to read 
> files larger than 2 GiB on 32 bit systems.
> 
> But please, don't send a new version. I do not accept patches with 
> windows-specific code.

Then os.h should be included first (or before the <c*> headers)? Would you 
accept os.h at all (or another construction to make syscalls replaceable), 
without the Windows specific functions filled in, i.e. only os_open(), 
os_read(), etc. for Unix? I then only needed to modify os.h for my Windows port.

> > I think that in all loops such as
> > for (i=0; i<vector<>::size(); i++)
> > i should be of the same type as size() returns. That's not int and not
> unsigned int either, but size_t.
> 
> If the size never exceeds what an unsigned int can contain, why should I 
> use size_t?

Because you can't tell for sure, I think. At least if you use size_t, it's more 
likely to be error free. Think of int being 32bit on a 64bit environment. 
size_t would be 64bit there. Windows 64bit has this.

You might want to reduce the type names used within ddrescue to avoid trouble 
with the header files on different platforms, and therefore not use size_t or 
time_t. Generally agreed, but as you must #include <time.h>, you also have 
time_t, so why not use it? time_t could be 64bit on a 32bit platform, and long 
being still 32bit. size_t should also be standard enough to be available on all 
platforms which have vector<>.

> > Every index access, i.e. anything within [] should be of type size_t, or
> at least unsigned.
> 
> Why? AFAIK, negative indices are generally valid as long as array limits 
> are not surpassed, being p[-1] equivalent to *(p-1).

Generally yes, but in ddrescue p[-1] is apparently not used. So in most places 
the change wouldn't cost anything. And because the index comes from some size() 
call, MSVC warns about the unsigned/signed conversion. 
Newer g++ might also warn. I think it's better to change the index to unsigned 
if easily possible instead of adding casts. But I can live with the warnings, 
i.e. leave everything as it is.

If you don't want Windows specific code within ddrescue, the most helpful thing 
for me would be the os.h (or equivalent) thing. And I would like to ask you for 
accepting such a thing. There might be more platforms with incompatible 
syscalls.

Regards,
Martin