[cfe-commits] [PATCH] Explicitly Build __builtin_va_list

[Jordan Rose]

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.

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

Thanks for working on this!
Jordan


On Jun 13, 2012, at 10:03 AM, Meador Inge wrote:

> (Doug, Jordan, sorry if you already received this.  My message to the list
> bounced the first time.)
> 
> Hi All,
> 
> As discussed here [1], I have been working on a patch to explicitly build the
> target specific __builtin_va_list type instead of just injecting a string into
> the preprocessor input.  This will provide a cleaner separation between
> preprocessor input and builtin types, will allow us to remove the special
> casing in 'Sema::ActOnTypedefNameDecl' to notify the AST context of
> __builtin_va_list, and will allow us to remove the extra tracking added for PR
> 12665 in r158085.
> 
> OK to commit?  If this is accepted, I will go backout the tracking portions of
> r158085 in another commit.
> 
> [1] http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120604/058597.html
> 
> -- 
> Meador Inge
> CodeSourcery / Mentor Embedded
> http://www.mentor.com/embedded-software
> <builtin-va-list.patch>