[llvm-commits] [llvm] r43261 - in /llvm/trunk: include/llvm/Bitcode/Serialization.h lib/Bitcode/Reader/Deserialize.cpp lib/Bitcode/Writer/Serialize.cpp

[Ted Kremenek]

On Oct 23, 2007, at 10:05 PM, Chris Lattner wrote:

>> Would it make sense to split this function up into a Serializer.h and
> Deserializer.h that only brings on one of these each respectively?
> Then Serialization.h would have the stuff that is truly common.
> This would also match the structure of your .cpp files.

Make sense.

>> +#include "llvm/ADT/SmallVector.h"
>> +#include <vector>
>> +
>> +namespace llvm {
>> +
>> +template <typename T> struct SerializeTrait;
>> +
>> +class Serializer {
>
> This needs a nice big doxygen comment above it explaining what it is.

Documentation, who needs that?  There aren't really any comments  
there, and you want doxygen comments!

>> +template <> struct SerializeTrait<bool>
>> +  : public SerializeIntTrait<bool,1> {};
>> +
>> +template <> struct SerializeTrait<char>
>> +  : public SerializeIntTrait<char,8> {};
>> +
>> +template <> struct SerializeTrait<short>
>> +  : public SerializeIntTrait<short,16> {};
>> +
>> +template <> struct SerializeTrait<unsigned>
>> +  : public SerializeIntTrait<unsigned,32> {};
>
> This won't work: the sizes of these types aren't guaranteed to be
> what you claim them to be.  You could use:
>
>> +template <> struct SerializeTrait<unsigned>
>> +  : public SerializeIntTrait<unsigned,sizeof(unsigned)*8> {};
>
> though.

Right.

> More significantly, I don't think you need a size in bits at all.
> Just allow the default vbr encoding to work for now, we can worry
> about optimizing them later, no?

These sizes are currently ignored.  I was thinking of using them as  
clues for abbreviations, etc.  I can simplify it until I know what the  
real optimization will be.

>> = 
>> =====================================================================
>> ========
>> --- llvm/trunk/lib/Bitcode/Writer/Serialize.cpp (added)
>> +++ llvm/trunk/lib/Bitcode/Writer/Serialize.cpp Tue Oct 23 16:29:33
>> 2007
>> @@ -0,0 +1,52 @@
>> +
>> +Serializer::Serializer(BitstreamWriter& stream, unsigned BlockID)
>> +  : Stream(stream), inBlock(BlockID >= 8) {
>> +
>> +  if (inBlock) Stream.EnterSubblock(8,3);
>
> This is obviously very early, but what is the magic number 8 here?
>
>
>> +void Serializer::EmitRecord() {
>> +  assert(Record.size() > 0 && "Cannot emit empty record.");
>> +  Stream.EmitRecord(8,Record);
>
> and here?

No particular reason for 8, other than the custom codes start at 8  
(which I know doesn't apply to block ids or records).  I just picked 8  
so that it wouldn't conflict with anything I knew of.  Clearly this  
must be changed eventually.

The bigger question is what will record IDs be used for?  I can  
envision uses for the block IDs (just sections in the file), but what  
about the records?