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

[Talin]

I'll work on this later today. One note: I'm not intentionally reverting
anything, rather it's that the code is changing out from under me (this
patch is already over a month old, a lot of code has changed in that time.)

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

>
> On Feb 9, 2010, at 4:28 PM, Talin wrote:
>
> ping...
>
>
> Hi Talin, sorry for the delay.  FWIW, it's usually best to trickle pieces
> of a feature in and build it up over time, otherwise your patch just gets
> larger and larger.
>
> 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".
>
> 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.
>
> +void ConstantUnion::replaceUsesOfWithOnConstant(Value *From, Value *To,
> +                                                 Use *U) {
> +  assert(false && "Implement replaceUsesOfWithOnConstant for unions");
> +}
> +
>
> Still not implemented?
>
> +UnionType *UnionType::get(const Type *type, ...) {
> +  va_list ap;
> +  std::vector<const llvm::Type*> UnionFields;
> +  va_start(ap, type);
>
> Please use smallvector.
>
> +bool UnionType::isValidElementType(const Type *ElemTy) {
> +  return !ElemTy->isVoidTy() && !ElemTy->isLabelTy() &&
> +         !ElemTy->isMetadataTy() && !isa<FunctionType>(ElemTy);
> +}
>
> Isn't there a better predicate somewhere?
>
> +LLVMTypeRef LLVMUnionTypeInContext(LLVMContextRef C, LLVMTypeRef
> *ElementTypes,
> +                           unsigned ElementCount) {
> +  std::vector<const Type*> Tys;
> +  for (LLVMTypeRef *I = ElementTypes,
> +
>
> indentation of unsigned and use smallvector.
>
>
> +/// ParseUnionType
> +///   TypeRec
> +///     ::= 'union' '{' '}'
> +///     ::= 'union' '{' TypeRec (',' TypeRec)* '}'
> +bool LLParser::ParseUnionType(PATypeHolder &Result) {
>
> Unions can't be empty, so the first grammar production isn't valid.
>
> Otherwise, it looks good, please send these updates and I will commit it
> for you.
>
> -Chris
>
>


-- 
-- Talin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20100210/9b858ec0/attachment.html>