[syslinux] [PATCH 02/23] com32/chain: comments, minor adjustments

[Michal Soltys]

On 2012-11-08 02:34, Shao Miller wrote:
>
> An alternative to this would be a final 'else' that bails with an error.
>   While unlikely, it _could_ be useful for chain.c32 to understand APM,
> some day.  That'll be even more useful once chain.c32 stuff is further
> librarized for use with future multiple FS support.  If APM support were
> introduced, this recent hunk would change back the way it was.
>
> .cut.
>
> I didn't see this until just now. :)  It seems like it might've been
> fine the way it was, given my opinion above.
>

Allright, will go back to earlier version.

>> diff --git a/com32/chain/utility.c b/com32/chain/utility.c
>> index bc5b81a..d40c0dd 100644
>> --- a/com32/chain/utility.c
>> +++ b/com32/chain/utility.c
>> -    (*dst)[0] = h;
>> -    (*dst)[1] = s | ((c & 0x300) >> 2);
>> -    (*dst)[2] = c;
>> +    (*dst)[0] = (uint8_t)h;
>> +    (*dst)[1] = (uint8_t)(s | ((c & 0x300) >> 2));
>> +    (*dst)[2] = (uint8_t)c;
>>   }
>>
>>   uint32_t get_file_lba(const char *filename)
>
> Was this producing warnings?  Those casts seem redundant, since the
> assignment expression has type 'uint8_t'.  It's a touch more cluttered,
> with those.

Only with -Wconversion (which is rather pedantic setting, beyond what 
-Wall and -Wextra set). I occasionally use that warning to catch some 
unexpected things that could be missed (e.g. in the past I found a 
function in disklib which was called with 64bit sector while argument 
was 32bit - check 578bd203ba04816) - but TBH this is generally one-time 
check.

Either way is fine with me - -Wconversion would require quite a few 
fixes throughout the whole code to not stop at -Werror, with irritating 
casts all over the places. Some additional uint8_t casts could be 
removed then (they were added in aa66e5191c5023e).

>> +int guid_is0(const struct guid *guid)
>> +{
>> +    return !*(const uint64_t *)guid && !*((const uint64_t *)guid + 1);
>> +}
>> +
>> +
>>   #endif
>>
>>   /* vim: set ts=8 sts=4 sw=4 noet: */
>>
>
> I know this is a move from earlier code ('static inline' seems like a
> good idea), but for 'guid_is0', why not test the individual members
> instead of re-interpreting the object representation as a different
> type?  There are only 4 members and there are 2 comparands, here.  That
> doesn't take much more code, and removes assumptions about object
> representation.
>

Ok, will change that - not much of a difference and certainly cleaner :)

As for the assumnption about object representation - well, the struct 
guid is ((packed)) and the syslinux sets -fno-strict-aliasing globally 
to permit that "officially" (though even without it, verifying for 0 
should be practically fine, even if formally undefined behavior).