[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