[cfe-commits] [PATCH] Explicitly Build __builtin_va_list

[Meador Inge]

On 06/14/2012 12:12 PM, Jordan Rose wrote:

> I like it! All I have are some nitpicks:
> 
> +  /// getBuiltinVaListKind - Returns the kind of __builtin_va_list
> +  /// type that should be implemented by this target.
> 
> s/implemented by/used with/, perhaps? The target isn't doing the implementing.

Fixed.

> +  switch (Kind) {
> +  default: llvm_unreachable("Unhandled builtin type kind");
> 
> Please put the llvm_unreachable outside the switch so that -Wswitch can give us warnings but GCC still won't whine at us.

Fixed.

> +  if (!BuiltinVaListDecl) {
> +    BuiltinVaListDecl = CreateVaListDecl(this, Target->getBuiltinVaListKind());
> +  }
> 
> Spurious {}, according to the LLVM coding style.

Fixed.

> +  if (Context.getBuiltinVaListDecl())
> +    DeclIDs[Context.getBuiltinVaListDecl()] = PREDEF_DECL_BUILTIN_VA_LIST_ID;
> +
> 
> Because you're using the accessor, you're forcing construction of the type. Either add a ShouldCreate flag (that can default to true) or access the instance variable directly like the other decls.

Fixed;  I access the variable directly.

> Again, I'm not an expert on this part of the code, so at least Doug should get a chance to review it before it goes in.
> 
> There's a bunch of other code that works around the existing __builtin_va_list hack besides r158085, but some of that may still need to exist since it can still show up in the TU. (But hopefully only when requested now.) Both the __int128 decls and the __builtin_va_list decls should probably be marked Implicit, as tested for in DeclPrinter.cpp.
> 
> (You're not required to track down all the existing hacks before getting the patch in; I just don't want them to stay around forever.)

I am happy to help track down the existing hacks after this gets applied.

> Thanks for working on this!

Thanks for the review!

New patch attached.

-- 
Meador Inge
CodeSourcery / Mentor Embedded
http://www.mentor.com/embedded-software
-------------- next part --------------
A non-text attachment was scrubbed...
Name: builtin-va-list-v2.patch
Type: text/x-patch
Size: 27833 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120615/52538450/attachment.bin>