[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

[Meador Inge]

On Thu, Jun 7, 2012 at 12:15 PM, Douglas Gregor <dgregor at apple.com> wrote:

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

OK, I went with the enumeration for now.

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

Will do.

Thanks for the feedback y'all.  I just sent out a patch including these updates.

-- 
# Meador