On Dec 22, 2007, at 5:28 PM, Chris Lattner wrote:
Please attach patches as attachments, not inline. If you're using 'mail', just name the file "whatever.patch" and this will happen. Until this happens, I can't properly review the patch as it's all wrapped and nastified :(

Odd, you must be using an old mail viewer... In modern viewers, it should come out just fine. I pretested cutting and pasting and ensured there were no problems with it. Oh well... What mail viewer do you use?

+/// LinkageSpecDecl - This represents a linkage specification.

+class LinkageSpecDecl : public Decl {

Please give an example of what a linkage spec is in the header. At some point, we probably want to make a DeclCXX.h file, but deferring this for now is fine.

+private:
+  /// Language - The language for this linkage specification.
+  Language language;
+  bool inside_braces;
+  Decl *D;

inside_braces is good for "remembering" the original source,

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?

but unless there is a client,

-ast-print uses that information to print the ast. Absent that information we can't faithfully reproduce the syntax tree. Do we care that the printed ast is wrong? Do we care if the ast isn't actually an ast?

I'd suggest not including it. How does your Decl structure handle stuff like:

extern "C" {
 int x;  int y;
}

work in progress... but roughly, an AST should have one linkage spec with two decls under it.

Is there just one LinkageSpecDecl?

Yes.  This choice was mainly driven by keeping the AST an ast.

+++ ./Parse/Parser.cpp  2007-12-21 13:58:52.000000000 -0800
@@ -378,6 +378,12 @@ Parser::DeclTy *Parser::ParseDeclaration
      return ParseObjCAtInterfaceDeclaration(AtLoc,
DS.getAttributes());
  }

+  if (Tok.is(tok::string_literal)
+      && DS.getStorageClassSpec() == DeclSpec::SCS_extern)
+    {
+      return ParseLinkage(Declarator::FileContext);

I'm not sure if this is sufficient:

It wasn't, but that was a known issue.

does this allow 'extern int "C"'  or 'extern inline "C"'

I'm sure...

The DeclSpec::getParsedSpecifiers() method can be used to make sure there is just a storage class specified.

Ah, perfect, I've fixed it and tested extern inline "C"; that is now rejected.

+Sema::DeclTy* Sema::ActOnLinkageSpec(SourceLocation Loc,
+                                    std::string Lang,
+

Please don't pass std::strings by-value, pass by const reference, or better yet, pass in the string as a const char* or something like that.

Ok, fixed.

Attachment: d.patch
Description: Binary data


_______________________________________________
cfe-dev mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

Reply via email to