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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
