[cfe-commits] r158085 - in /cfe/trunk: include/clang/Basic/DiagnosticParseKinds.td include/clang/Basic/SourceManager.h include/clang/Parse/Parser.h lib/Basic/SourceManager.cpp lib/Lex/Preprocessor.cpp lib/Parse/ParseAST.cpp lib/Parse/Parser.cpp t

[Douglas Gregor]

On Jun 7, 2012, at 9:55 AM, Jordan Rose wrote:

> 
> On Jun 6, 2012, at 18:49 , Meador Inge <meadori at gmail.com> wrote:
> 
>> On Wed, Jun 6, 2012 at 12:25 PM, Jordan Rose <jordan_rose at apple.com> wrote:
>>> Author: jrose
>>> Date: Wed Jun  6 12:25:21 2012
>>> New Revision: 158085
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=158085&view=rev
>>> Log:
>>> Add pedantic warning -Wempty-translation-unit (C11 6.9p1).
>>> 
>>> In standard C since C89, a 'translation-unit' is syntactically defined to have
>>> at least one "external-declaration", which is either a decl or a function
>>> definition. In Clang the latter gives us a declaration as well.
>>> 
>>> The tricky bit about this warning is that our predefines can contain external
>>> declarations (__builtin_va_list and the 128-bit integer types). Therefore our
>>> AST parser now makes sure we have at least one declaration that doesn't come
>>> from the predefines buffer.
>>> 
>>> Also, remove bogus warning about empty source files. This doesn't catch source
>>> files that only contain comments, and never fired anyway because of our
>>> predefines.
>>> 
>>> PR12665 and <rdar://problem/9165548>
>> 
>> I agree with the decision to track the pre-defined decls as a
>> short-term solution.  Longer term I still think injecting strings into
>> the preprocessor input like this is still gross because: (1)
>> 'InitializePredefinedMacros' is for initializing, uh, macros and
>> __builtin_va_list is not a macro, (2) there isn't a clean separation
>> between all of the builtin types and the builtin preprocessor input,
>> (3) it causes issue like PR 12665 that we have to work around by doing
>> extra unnecessary tracking, and (4) there is special casing in
>> 'Sema::ActOnTypedefNameDecl' to notify the AST context of the types
>> that can be removed.
>> 
>> Attached is a work in progress patch where I move the
>> __builtin_va_list type construction to the TargetInfo classes.
>> However, there are some drawbacks to this approach at this point in
>> time: (1) it is a breaking change for the AST serialization format and
>> (2) libclangBasic now depends on libclangAST thus introducing a cyclic
>> dependency.
>> 
>> Anyway, I am glad PR 12665 is fixed and I just wanted to share the
>> __builtin_va_list type construction work I have been playing with.
> 
> I totally agree with you that this is a hack, and that defining __builtin_va_list textually is a worse hack. Again, since this is a warning that doesn't really fire on real code, it wasn't worth ripping up the existing hack just for this. But you're right that we probably should in the long run.
> 
> The cyclic dependency kills the idea of just putting the manual AST-building in each TargetInfo instance, but maybe we could have an enum like this?
> 
> enum VAListKind {
>  VALK_VoidPtr,
>  VALK_CharPtr,
>  VALK_FourIntArray,
>  VALK_ELFStruct,
>  VALK_DarwinStruct
> };
> 
> It's still ugly, and it means that ASTContext has to know about all the possibilities, but at least this way it /doesn't/ leak out into the rest of the world. Or at least not as much.

I think this solution is perfectly reasonable. If we're ever forced to generalize further, we can do something like we do with built-in functions, and have a mini-language that's expressive enough to describe these data structures.

> As far as changing serialization, there are a couple options for backwards compatibility. First, leave the enum in there as unused. Second, continue serializing __builtin_va_list even though we can reconstruct it later. This gives us the option to check for incompatible definitions, though I don't think that actually happens right now, in case the serialized AST came from a different target. (Although we should already be complaining about that…)
> 
> Third, go ahead and break backwards compatibility. When we add new attributes to Expr and Decl we have to change the serialization code. But I don't want to be the one to make the call there.


Go ahead and break backward compatibility for serialization. We do it all the time.

	- Doug