On Thu, Jun 7, 2012 at 12:15 PM, Douglas Gregor <[email protected]> wrote:
> On Jun 7, 2012, at 9:55 AM, Jordan Rose wrote: > >> >> On Jun 6, 2012, at 18:49 , Meador Inge <[email protected]> wrote: >> >>> On Wed, Jun 6, 2012 at 12:25 PM, Jordan Rose <[email protected]> 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 _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
