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

[Chris Lattner]

> URL: http://llvm.org/viewvc/llvm-project?rev=43261&view=rev
> Log:
> Added preliminary implementation of generic object serialization to  
> bitcode.
>
> ====================================================================== 
> ========
> --- llvm/trunk/include/llvm/Bitcode/Serialization.h (added)
> +++ llvm/trunk/include/llvm/Bitcode/Serialization.h Tue Oct 23  
> 16:29:33 2007
> @@ -0,0 +1,115 @@
> + //=- Serialization.h - Generic Object Serialization to Bitcode --- 
> *- C++ -*-=//

A critical flaw is revealed!  you have an extra space at the start of  
the line.  :)

> +#ifndef LLVM_BITCODE_SERIALIZE
> +#define LLVM_BITCODE_SERIALIZE
> +
> +#include "llvm/Bitcode/BitstreamWriter.h"
> +#include "llvm/Bitcode/BitstreamReader.h"

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.

> +#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.

> +  BitstreamWriter& Stream;
> +  SmallVector<uint64_t,10> Record;
> +  bool inBlock;
> +public:
> +  Serializer(BitstreamWriter& stream, unsigned BlockID = 0);
> +  ~Serializer();
> +
> +  template <typename T>
> +  inline void Emit(const T& X) { SerializeTrait<T>::Serialize 
> (*this,X); }
> +
> +  void EmitInt(unsigned X, unsigned bits);
> +  void EmitCString(const char* cstr);
> +
> +  void Flush() { if (inRecord()) EmitRecord(); }
> +
> +private:
> +  void EmitRecord();
> +  inline bool inRecord() { return Record.size() > 0; }
> +};
> +
> +
> +class Deserializer {

Likewise, needs a comment.

>
> +template <typename uintty, unsigned Bits>
> +struct SerializeIntTrait {
> +  static inline void Serialize(Serializer& S, uintty X) {
> +    S.EmitInt(X,Bits);
> +  }
> +
> +  static inline void Deserialize(Deserializer& S, uintty& X) {
> +    X = (uintty) S.ReadInt(Bits);
> +  }
> +};

Ok.

> +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.

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?


> ====================================================================== 
> ========
> --- 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?

-Chris