[cfe-commits] [PATCH] Explicitly Build __builtin_va_list

[Douglas Gregor]

On Jun 14, 2012, at 10:12 AM, 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.
> 
> 
> +  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.
> 
> 
> +  if (!BuiltinVaListDecl) {
> +    BuiltinVaListDecl = CreateVaListDecl(this, Target->getBuiltinVaListKind());
> +  }
> 
> Spurious {}, according to the LLVM coding style.
> 
> 
> +  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.

Right; just poke at the instance variable directly, because that's what we're doing for the others.

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

Yes, they should be marked implicit. That'll keep a number of other places in the code from treating them as user-written code.

Patch looks great. Please feel free to commit once you've addressed Jordan's comments, thanks!

	- Doug