Re: [libvirt] [PATCHv2] storage_encryption: silence clang warning
Eric Blake wrote:
On 05/10/2010 01:02 PM, Jim Meyering wrote:
> Eric Blake wrote:
>> For printf("%*s",foo,bar), clang complains if foo is not int:
>>
>> warning: field width should have type 'int', but argument has
>> type 'unsigned int' [-Wformat]
>>
>> virStorageEncryptionSecretFormat(virBufferPtr buf,
>> virStorageEncryptionSecretPtr secret,
>> - unsigned int indent)
>> + int indent)
>
> "unsigned int" sounds like the right type to me, since
> "indent" never goes negative. And using the unsigned type
> seems to be in line with policy in HACKING:
>
> If a variable is counting something, be sure to declare it with an
> unsigned type.
I'm not lobbying for further change, but merely
making sure you see where I'm coming from.
But printf("%*s", indent, string) would indeed behave
differently if
indent goes negative (if signed) or larger than INT_MAX (if unsigned),
That's true, but the meaning of the variable's type is important
and affects two interfaces, whereas the cast is local.
Making it signed via "int" should make the reader/reviewer think
a negative value is an option. Here, it is not.
so clang's warning is realistic, and worth silencing in our quest
to get
No argument there.
My point was that a cast keeps the ugly compromise local to the
unfortunate interface (printf field width) that it accommodates,
rather than propagating it to another interface and even another file.
a clean clang build. About the only other thing I could think of to
do
that would avoid confusion and also to avoid casts is to keep the public
interface with unsigned int, then add an intermediate helper variable:
int real_indent = indent;
and use real_indent in the printf call, but that seems like overkill.
Using the cast is the lesser evil ;)
>
> Of course, avoiding casts is good, too, but IMHO, not if
> it makes us obfuscate (even ever so slightly) the types we use.
Well, the real point of this patch was to silence a compiler warning
(not that we'd ever planning on passing an indent > 2G). Given Dave's
ACK, I went ahead and applied v2 as proposed, even if it does slightly
obfuscate the usage.
Warning removal is important.
But it can end up being counterproductive if not done carefully.