[cfe-dev] extern "C" support

[Chris Lattner]

On Jan 8, 2008, at 7:10 PM, Mike Stump 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.

Gotcha.  We definitely have an AST, we just keeps some parse tree'y  
features in it :).

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

ok :)

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

Thanks!

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

Hehe, yeah :)

> Let's try this version:

This is looking very nice.  Some last remaining nit-picky details:

+Parser::DeclTy *Parser::ParseLinkage(unsigned Context) {
+  assert(Tok.is(tok::string_literal) && "Not a stringliteral!");
+  llvm::SmallVector<char, 8> LangBuffer;
+  LangBuffer.resize(Tok.getLength());    // LangBuffer is guaranteed  
to be big enough.


Please fit to 80 cols.

+/// CodeGenLinkageSpec - Emit the specified linkage space to LLVM.
+void clang::CodeGen::CodeGenLinkageSpec(CodeGenModule *Builder,  
LinkageSpecDecl *LS) {

likewise

@@ -156,7 +157,8 @@ void Decl::PrintStats() {
  	      nFileVars*sizeof(FileVarDecl)+nParmVars*sizeof(ParmVarDecl)+
  	      nFieldDecls*sizeof(FieldDecl)+nSUC*sizeof(RecordDecl)+
  	      nEnumDecls*sizeof(EnumDecl) 
+nEnumConst*sizeof(EnumConstantDecl)+
-	      nTypedef*sizeof(TypedefDecl)) /* FIXME: add ObjC decls */);
+	      nTypedef*sizeof(TypedefDecl) 
+nLinkageSpecDecl*sizeof(LinkageSpecDecl))
+	  /* FIXME: add ObjC decls */);
  }

likewise.

+void LinkageSpecDecl::EmitInRec(Serializer& S) const {
+  Decl::EmitInRec(S);
+  S.EmitInt(getLanguage());
+  S.EmitPtr(D);
+}

Emitting the language enum as part of the serialization format  
implicitly makes the enum value part of the serialized encoding.  I'm  
fine with this, but please add a comment about this issue above the  
enum definition:

+class LinkageSpecDecl : public Decl {
+public:
+  enum LanguageIDs { lang_cxx, lang_c };

How about:
    // Note that changing these enum values will break compatibility  
with serialized ASTs.
+  enum LanguageIDs { lang_cxx, lang_c };

or something.


+void DeclPrinter::PrintLinkageSpec(LinkageSpecDecl *LS) {
+  std::string l;
+  if (LS->getLanguage() == LinkageSpecDecl::lang_c)
+    l = "C";
+  else if (LS->getLanguage() == LinkageSpecDecl::lang_cxx)
+    l = "C++";

I'd declare "l" as const char* instead of std::string, to avoid a  
malloc/free.  The performance of the declprinter isn't very important,  
but it's good to avoid gratuitous heap thrashing :).


+class LinkageSpecDecl : public Decl {
...
+  Decl *getDecl() { return D; }

Please add a const version of the accessor as well:
+  const Decl *getDecl() const { return D; }


+++ ./include/clang/Parse/Action.h	2008-01-08 18:59:11.000000000 -0800
@@ -154,6 +154,12 @@ public:
+  virtual DeclTy *ActOnLinkageSpec(SourceLocation Loc, SourceLocation  
LBrace,
+				   SourceLocation RBrace, const char *,
+				   unsigned, DeclTy *D) {

Please give a name to the const char* and unsigned argument so that it  
is more obvious what is expected.  The names you have in Sema.h are  
good.



+/// ParseLinkage - We knnow that the current token is a string_literal

knnow -> know

+/// and just before that, that extern was seen.
+///
+///       linkage-specification: [C++ 7.5p2: dcl.link]
+///         extern string-literal { declaration-seq opt }
+///         extern string-literal declaration

Very nice, but please follow the form of the other parser routines,  
for example 'extern' instead of extern, '{' instead of {, etc.


+  // std::string lang = PP.getSpelling(Tok);

This comment is dead, plz remove.

+  if (D)
+    return Actions.ActOnLinkageSpec(Loc, LBrace, RBrace, LangBufPtr,  
StrSize, D);
+
+  return 0;

In this case it really doesn't matter much, but I prefer to have the  
code structured (where possible) so that errors exit the function and  
that the fall-through is the normal case.  Please use:

+  if (!D) return 0;
+
+  return Actions.ActOnLinkageSpec(Loc, LBrace, RBrace, LangBufPtr,  
StrSize, D);

Again, it doesn't actually matter much for this case, but keeping the  
code consistent is good.


+++ ./Parse/Parser.cpp	2008-01-08 18:59:11.000000000 -0800
@@ -386,6 +386,11 @@ Parser::DeclTy *Parser::ParseDeclaration

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

Please add a comment explaining the logic here.  Something like:
   // If the declspec consisted only of 'extern' and we have a string  
literal following it, this must be a C++ linkage specifier like  
'extern "C"'.


+  if (strncmp (Lang, "\"C\"", StrSize) == 0)
+    Language = LinkageSpecDecl::lang_c;
+  else if (strncmp (Lang, "\"C++\"", StrSize) == 0)
+    Language = LinkageSpecDecl::lang_cxx;

I'm not sure if strncmp is the right thing here.  Will it work if  
"Lang" is not null terminated?  Also, please format the code as  
"strncmp(" instead of "strncmp (".

If strncmp won't work, one way to go would be:

   if (StrSize == 3 && memcmp(Lang, "\"C\"", 3) == 0) ...

Finally, should 'extern "c"' be allowed?

Thanks Mike, this is converging very nicely!

-Chris