[llvm-commits] [llvm] r76759 - in /llvm/trunk: include/llvm/Bitcode/LLVMBitCodes.h include/llvm/Constants.h include/llvm/MDNode.h include/llvm/Value.h lib/AsmParser/LLParser.cpp lib/AsmParser/LLParser.h lib/Bitcode/Reader/BitcodeReader.cpp lib/Bitcode/Reader/BitcodeReader.h lib/Bitcode/Writer/BitcodeWriter.cpp lib/Bitcode/Writer/ValueEnumerator.cpp lib/VMCore/AsmWriter.cpp lib/VMCore/Constants.cpp lib/VMCore/Value.cpp test/Feature/embeddedmetadata.ll test/Feature/mdnode.ll tools/llvm-bcanalyzer/llvm-bcanalyzer.cpp

[Devang Patel]

On Jul 22, 2009, at 5:02 PM, Chris Lattner wrote:

> On Jul 22, 2009, at 10:43 AM, Devang Patel wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=76759&view=rev
>> Log:
>> Introduce MetadataBase, a base class for MDString and MDNode.
>> Derive MDString directly from MetadataBase.
>> Introduce new bitcode block to hold metadata.
>
> Nice metadata shouldn't be "isa<Constant>".
>
>> +++ llvm/trunk/include/llvm/MDNode.h Wed Jul 22 12:43:22 2009
>> @@ -31,6 +31,65 @@
>> namespace llvm {
>>
>> // 
>> = 
>> = 
>> = 
>> ----------------------------------------------------------------------= 
>> ==//
>> +// MetadataBase  - A base class for MDNode and MDString.
>> +class MetadataBase : public Value {
>
> Please add a significant doxygen comment here.

ok

>
>> +public:
>> +  MetadataBase(const Type *Ty, unsigned scid)
>> +    : Value(Ty, scid) {}
>
> This shouldn't have a public ctor, it should be protected because it  
> is an abstract class.

ok

>
>> +// 
>> = 
>> = 
>> = 
>> ----------------------------------------------------------------------= 
>> ==//
>> +/// MDString - a single uniqued string.
>> +/// These are used to efficiently contain a byte sequence for  
>> metadata.
>> +///
>> +class MDString : public MetadataBase {
>> +  MDString(const MDString &);            // DO NOT IMPLEMENT
>> +
>> +  const char *StrBegin, *StrEnd;
>
> Storing start+end is somewhat unconventional.  Why not start+length  
> instead?

I do not know. Just cut-n-pasted from existing MDString class.

>
>> +  friend class LLVMContextImpl;
>> +
>> +public:
>> +  MDString(const char *begin, const char *end)
>> +    : MetadataBase(Type::MetadataTy, Value::MDStringVal),
>> +      StrBegin(begin), StrEnd(end) {}
>
> MDStrings should be uniqued, right?  If so, this ctor should be  
> private.

There is not getMDString(). The LLVMContext manages MDString cache and  
uses this constructor.

>
>> +
>> +  intptr_t size() const { return StrEnd - StrBegin; }
>
> Is size() the best name for this?  Maybe length would be better?

ok. cut-n-paste from existing MDString class.

> It should return unsigned, not intptr_t.  'intptr_t' is an  
> implementation detail.

ok

Thanks!
-
Devang