[cfe-dev] extern "C" support

[Mike Stump]

On Jan 9, 2008, at 1:36 PM, Chris Lattner wrote:
> This is looking very nice.  Some last remaining nit-picky details:

Time will tell if these are truly the last of the nit-pics...  :-)

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

Done.

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

Done.

> @@ -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 */);
> }

Done.

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

Done.  Hum, yeah, would be nice to have a nice clean separation  
between the two.  For an abi, I think I'd rather have the value chosen  
by the dwarf standard, I think they both enumerate languages and  
select values.  [ pause ]  Ok, I fixed up the code.

> I'd declare "l" as const char*

Done.

> Please add a const version of the accessor as well:

Done.

> Please give a name to the const char* and unsigned argument so that  
> it is more obvious what is expected.

Done.

> knnow -> know

Fixed.

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

Done.

> Please use:
> +  if (!D) return 0;

Done.

> Please add a comment explaining the logic here.

Done.

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

Uhm, this is why strncmp was invented!?  Yes.  strncmp only fails when  
languages are defined to have '\0' in their names.  When that happens,  
we have bigger problems.

>  Also, please format the code as "strncmp(" instead of "strncmp (".

Done.

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

No.

Ok, We'll try this one...

-------------- next part --------------
A non-text attachment was scrubbed...
Name: externc-3.patch
Type: application/octet-stream
Size: 14049 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20080110/be569a0b/attachment.obj>
-------------- next part --------------