[LLVMdev] [patch] Union Types - work in progress

[Talin]

OK here's a new patch. Additional comments below.

On Wed, Feb 10, 2010 at 10:57 AM, Chris Lattner <clattner at apple.com> wrote:

>
> LangRef.html:
>
> +  <dt><b>Union constants</b></dt>
> +  <dd>Union constants are represented with notation similar to a structure
> with
> +      a single element - that is, a single typed element surrounded
> +      by braces (<tt>{}</tt>)).  For example: "<tt>{ i32 4 }</tt>".  A
> +      single-element constant struct can be implicitly converted to a
> +      <a href="#t_union">union type</a> as long as the type of the struct
> +      element matches the type of one of the union members.</dd>
> +
>
> It's a minor point, but I'd avoid the term "implicitly converted".
>

done

>
> Constants.cpp:
>
> -  assert(Idx->getType()->isInteger(32) &&
> +  assert(Idx->getType() == Type::getInt32Ty(Val->getContext()) &&
>
> You're reverting these changes, please don't.  There are a couple instances
> of this.
>

Didn't mean to :) Hope that the train hasn't moved too far while I was
editing this.

>
> +void ConstantUnion::replaceUsesOfWithOnConstant(Value *From, Value *To,
> +                                                 Use *U) {
> +  assert(false && "Implement replaceUsesOfWithOnConstant for unions");
> +}
> +
>
> Still not implemented?
>
> Not in this patch - as you say, it's too large already.


> +UnionType *UnionType::get(const Type *type, ...) {
> +  va_list ap;
> +  std::vector<const llvm::Type*> UnionFields;
> +  va_start(ap, type);
>
> Please use smallvector.
>

Done - although I was just copying from what Struct does.

>
> +bool UnionType::isValidElementType(const Type *ElemTy) {
> +  return !ElemTy->isVoidTy() && !ElemTy->isLabelTy() &&
> +         !ElemTy->isMetadataTy() && !isa<FunctionType>(ElemTy);
> +}
>
> Isn't there a better predicate somewhere?
>

Apparently there is now. Done.

>
> +LLVMTypeRef LLVMUnionTypeInContext(LLVMContextRef C, LLVMTypeRef
> *ElementTypes,
> +                           unsigned ElementCount) {
> +  std::vector<const Type*> Tys;
> +  for (LLVMTypeRef *I = ElementTypes,
> +
>
> indentation of unsigned and use smallvector.
>
> Done.

>
> +/// ParseUnionType
> +///   TypeRec
> +///     ::= 'union' '{' '}'
> +///     ::= 'union' '{' TypeRec (',' TypeRec)* '}'
> +bool LLParser::ParseUnionType(PATypeHolder &Result) {
>
> Unions can't be empty, so the first grammar production isn't valid.
>

Done.

>
> Otherwise, it looks good, please send these updates and I will commit it
> for you.
>
> As much as it pains me to say anything that would delay this getting
committed, it might make sense to apply this patch after the code freeze -
since the feature isn't going to be finished in time for 2.7.

-- 
-- Talin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100211/44b3d0a8/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: union.patch
Type: application/octet-stream
Size: 48234 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100211/44b3d0a8/attachment.obj>