On Jan 6, 2008, at 1:35 PM, Chris Lattner wrote:
On Jan 2, 2008, at 2:22 PM, Mike Stump wrote:On Dec 22, 2007, at 5:28 PM, Chris Lattner wrote:I did that merely because we're calling them ASTs and because we form ParenExprs. If we didn't want ASTs, we could get rid of ParenExprs?I don't understand what you're saying here. ASTs can take many forms.
Sorry, the eyes read AST, but the brain was thinking parse trees, due to ParenExprs.
You seem to be using an overly constrained definition of what an AST is :).
:-) Yeah, I agree, we don't need to preserve the parse tree in the AST!
In this specific case, I'd be fine with -ast-print always emitting {}'s for example.
Done.
Please add FIXME markers to everything you *know* is incomplete or incorrect.
Ok.
Many of the changes to AST/Decl.cpp and the change to Type.h are unrelated to this patch. It would be good to split them out as they aren't controversial at all :)
I sent the Type.h separately. I'm holding the other hostage. :-) Oops, it went missing in the Objc ObjC rewrite, oh well...
+void clang::CodeGen::CodeGenLinkageSpec(CodeGenModule *B, LinkageSpecDecl *LS) {+ +}Please have this emit an unsupported warning or something (see CodeGenModule::WarnUnsupported) so that we know when this is happening.
Gosh, none of the existing ones could be used, and I didn't want to learn how to do all that... [ pause ] Oh well. :-) Done.
Also, please add a FIXME.
Done.
+/// LinkageSpecDecl - This represents a linkage specification.
+class LinkageSpecDecl : public Decl {
As I requested previously, please add an example.
"Language language" is strange. I'd suggest renaming the enum to LanguageIDs or something.
Done.
+ bool inside_braces; If you want to keep this
Gone.
How about getDecls() ?
Done.
Please stay in 80 columns and use mixed caps instead of _'s.
Done.
Ok, but please format with the llvm style (see surrounding code) which has the &&'s on the previous line, no space before the (), and either drop the {} or make them K&R style.
Done.
Likewise in the implementation of ParseLinkage.
Done.
+// C++ 7.5p2
+Parser::DeclTy *Parser::ParseLinkage(unsigned Context) {
+ std::string lang = PP.getSpelling(Tok);
Please add an assertion that the Tok.is(stringliteral), and a
comment above the method, like all other parser methods have.
Done.
Also, this is the expensive form of getSpelling, [ ... ] Instead, please use something like this:
Done.
In addition to the formatting issues above, please rearrange this to make the small case come first.
Done.
Also, RBrace is dead, so you can just remove its declaration.
Done....
Better yet, you could pass *it* into ActOnLinkageSpec instead of 'inside_brace'.
Sure, done.
+ return Actions.ActOnLinkageSpec(Loc, lang.c_str(), saw_braces, D); Can D be null here?
Sure.
If so, you should check for it and return an error instead of building an invalid AST node.
Done.
+Sema::DeclTy* Sema::ActOnLinkageSpec(SourceLocation Loc,
+ const char *Lang,
+ bool inside_brace,
+ DeclTy *D) {
This looks good, but please clean up the formatting above and add a
fixme if there are semantic checks missing :)
Well, almost all of them are missing... :-) Added a FIXME. Let's try this version:
externc-2.patch
Description: Binary data
_______________________________________________ cfe-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
