[llvm-commits] [llvm] r162277 - /llvm/trunk/include/llvm/Bitcode/BitstreamWriter.h

[Duncan Sands]

Hi David,

>> --- llvm/trunk/include/llvm/Bitcode/BitstreamWriter.h (original)
>> +++ llvm/trunk/include/llvm/Bitcode/BitstreamWriter.h Tue Aug 21 08:47:25 2012
>> @@ -155,6 +155,7 @@
>>     }
>>
>>     void EmitVBR(uint32_t Val, unsigned NumBits) {
>> +    assert(NumBits <= 32 && "Too many bits to emit!");
>>       uint32_t Threshold = 1U << (NumBits-1);
>
> I don't think this one will warn anyway, since it's assigned to
> uint32_t, but the assert's probably not a bad idea anyway.

yes, it wasn't needed but seemed like a good idea.

>
>>
>>       // Emit the bits with VBR encoding, NumBits-1 bits at a time.
>> @@ -167,10 +168,11 @@
>>     }
>>
>>     void EmitVBR64(uint64_t Val, unsigned NumBits) {
>> +    assert(NumBits <= 32 && "Too many bits to emit!");
>>       if ((uint32_t)Val == Val)
>>         return EmitVBR((uint32_t)Val, NumBits);
>>
>> -    uint64_t Threshold = 1U << (NumBits-1);
>> +    uint64_t Threshold = uint64_t(1U << (NumBits-1));
>
> Rather than adding the cast here (I've no idea if that silences this
> static analysis tool) why not just change the type to uint32_t? That
> appropriately represents its range and it'll be promoted to uint64_t
> for the comparison below anyway, as far as I can see.

That does seem neater, thanks for the suggestion.  Commit 162286.

Ciao, Duncan.