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...
externc-3.patch
Description: Binary data
_______________________________________________ cfe-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
