[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

[Jordan Rose]

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.

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.