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.)<div>

<div><br><div class="gmail_quote">On Wed, Feb 10, 2010 at 10:57 AM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com">clattner@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">

<div style="word-wrap:break-word"><br><div><div>On Feb 9, 2010, at 4:28 PM, Talin wrote:</div><br><blockquote type="cite">ping...<br></blockquote><div><br></div><div>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.</div>

<div><br></div><div>LangRef.html:</div><div><br></div><div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+  <dt><b>Union constants</b></dt></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+  <dd>Union constants are represented with notation similar to a structure with</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+      a single element - that is, a single typed element surrounded</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+      by braces (<tt>{}</tt>)).  For example: "<tt>{ i32 4 }</tt>".  A</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+      single-element constant struct can be implicitly converted to a</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+      <a href="#t_union">union type</a> as long as the type of the struct</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+      element matches the type of one of the union members.</dd></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"><br></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">It's a minor point, but I'd avoid the term "implicitly converted".</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"><br></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">Constants.cpp:</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

<br></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">-  assert(Idx->getType()->isInteger(32) &&</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+  assert(Idx->getType() == Type::getInt32Ty(Val->getContext()) &&</div><div><br></div></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

You're reverting these changes, please don't.  There are a couple instances of this.</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"><br></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+void ConstantUnion::replaceUsesOfWithOnConstant(Value *From, Value *To,</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+                                                 Use *U) {</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+  assert(false && "Implement replaceUsesOfWithOnConstant for unions");</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+}</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+</div><div><br></div><div>Still not implemented?</div>

</div></div><div><br></div><div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+UnionType *UnionType::get(const Type *type, ...) {</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+  va_list ap;</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+  std::vector<const llvm::Type*> UnionFields;</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+  va_start(ap, type);</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"><br></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">Please use smallvector.</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"><br></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px"><div class="im"><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+bool UnionType::isValidElementType(const Type *ElemTy) {</div></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+  return !ElemTy->isVoidTy() && !ElemTy->isLabelTy() &&</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+         !ElemTy->isMetadataTy() && !isa<FunctionType>(ElemTy);</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+}</div><div><br></div><div>Isn't there a better predicate somewhere?</div><div><br></div><div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+LLVMTypeRef LLVMUnionTypeInContext(LLVMContextRef C, LLVMTypeRef *ElementTypes,</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+                           unsigned ElementCount) {</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+  std::vector<const Type*> Tys;</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+  for (LLVMTypeRef *I = ElementTypes,</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

<br></div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">indentation of unsigned and use smallvector.</div></div></div></div><div><br></div><div><br></div><div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+/// ParseUnionType</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+///   TypeRec</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+///     ::= 'union' '{' '}'</div>

<div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">+///     ::= 'union' '{' TypeRec (',' TypeRec)* '}'</div><div style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0px">

+bool LLParser::ParseUnionType(PATypeHolder &Result) {</div><div><font face="Inconsolata" size="3"><span style="font-size:13px"><br></span></font></div><div><font face="Inconsolata" size="3"><span style="font-size:13px">Unions can't be empty, so the first grammar production isn't valid.</span></font></div>

<div><font face="Inconsolata" size="3"><span style="font-size:13px"><br></span></font></div><div><font face="Inconsolata" size="3"><span style="font-size:13px">Otherwise, it looks good, please send these updates and I will commit it for you.</span></font></div>

<div><font face="Inconsolata" size="3"><span style="font-size:13px"><br></span></font></div><font color="#888888"><div><font face="Inconsolata" size="3"><span style="font-size:13px">-Chris</span></font></div><div><font face="Inconsolata" size="3"><span style="font-size:13px"><br>

</span></font></div></font></div></div></div></blockquote></div><br><br clear="all"><br>-- <br>-- Talin<br>
</div></div>