[syslinux] [PATCH 1/3] lib: fix compilation warnings from MS's WDK

Pete Batard pete at akeo.ie
Wed Feb 15 03:13:23 PST 2012


On 2012.02.15 01:09, H. Peter Anvin wrote:
> On 02/14/2012 06:03 AM, Pete Batard wrote:
>> libfat and libinstaller compile fine using the Windows Driver Kit's
>> compiler, except for a few warnings that are addressed with this patch.
>
> Hmm... I think the workarounds are probably uglier than the warnings,
> and more likely to cause actual problems; casts are typically a very bad
> thing.

I apologize if this sounds confrontational, but I'm afraid I have to 
call bullshit on that.

1. Beauty/ugliness is now the prime consideration for writing code?
Can you please define beauty then, so that we have a reference of what 
exactly we should aim for? Else, I'm afraid I can only continue to 
consider beauty/ugliness as an entirely subjective matter, with a 
definition that varies greatly from one person to the next, and 
therefore hardly one I would expect as the first line to dismiss a patch.
Personally, I think leaving a compiler produce warnings is a lot uglier 
than having casts in the code.

2. One thing I didn't mention is that, by default, the WDK treats 
warnings as errors, which also means that, by default, libinstaller and 
libfat will fail to compile on WDK. But even if WDK didn't have its 
-Werror turned on, it isn't that different from developers ensuring 
warnings are eliminated so that actual errors or potential problems are 
distinguishable more easily.
If these warnings were produced during default gcc compilation, would 
you want to eliminate them or leave them?

3. Please highlight the potential problems that you foresee from these 
casts.
The set_16_sl(), set_32() very much expect a 16/32 bit parameter. If 
this was ever not to be the case, then their names would have to be 
changed, since it wouldn't reflect the size of the parameter any longer. 
Hence none of 16/32 bit casts we do in these calls can be expected to 
cause a problem in the long run.
Likewise, the cast of size_t to int on strlen can hardly be expected to 
cause an issue, unless we ever expect to be dealing with paths and 
volume names that are larger than 2 GB at some stage...
With regards to 'tag' and 'size', they are truncated from int to 
uint8_t, after having actively been checked to be in the valid uint8_t 
range. How then are we likely to introduce an issue with the cast again?
Which leaves only the casts on cluster computation.
Do you foresee cluster and nclusters becoming 64 bit values in the future?

4. There's hardly any piece of C code that doesn't use cast, and with 
good reason. One may want to do computation in format that is different 
from the end result, or a function call, such as strlen, may return a 
type (size_t, which may be 64 bit) that isn't suited for the desired 
intent (eg. dealing with strings that are limited in size). I think that 
quite a few developers would disagree that casts are "typically a very 
bad thing". They should be avoided when possible yes, but declaring them 
as typically very bad is quite a long stretch...

Regards,

/Pete






More information about the Syslinux mailing list