rjmccall added inline comments.
================ Comment at: lib/CodeGen/CGObjCGNU.cpp:961 + GV->setSection(Section); + return GV; + } ---------------- I'd encourage you to use ConstantBuilder whenever you would want to use this. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:966 + + std::string Str = SL->getString().str(); + CharUnits Align = CGM.getPointerAlign(); ---------------- Why copy the string? Also, you don't actually use it until later. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:977 + if ((CGM.getTarget().getPointerWidth(0) == 64) && + (SL->getLength() < 9) && !isNonASCII) { + // Tiny strings are (roughly): ---------------- Please hoist `SL->getLength()` into a variable. Also, consider breaking this bit of code (maybe just building a `uint64_t`?) into a separate function. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:992 + // }; + // With a tag value of 4. + uint64_t str = 0; ---------------- It's weird to give a struct like this and have it only be right on a *big*-endian target. You might also consider just using `uint64_t`. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:996 + for (unsigned i=0 ; i<SL->getLength() ; i++) + str |= ((uint64_t)SL->getCodeUnit(i)) << (57 - (i*7)); + // Fill in the length ---------------- I think this might be clearer as `64 - 7 - (i*7)`. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:1039 + if (isNonASCII) { + unsigned NumBytes = Str.size(); + SmallVector<llvm::UTF16, 128> ToBuf(NumBytes + 1); // +1 for ending nulls. ---------------- This is a very misleading variable name. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:1040 + unsigned NumBytes = Str.size(); + SmallVector<llvm::UTF16, 128> ToBuf(NumBytes + 1); // +1 for ending nulls. + const llvm::UTF8 *FromPtr = (const llvm::UTF8 *)Str.data(); ---------------- You should leave a comment saying that it's safe to expand an N-code-unit UTF-8 string into an N-code-unit UTF-16 buffer because UTF-16 never uses more code units than UTF-8. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:1077 + std::replace(StringName.begin(), StringName.end(), + '@', '\1'); + auto *ObjCStrGV = ---------------- Is `@` really the only illegal character in ELF symbol names? Surely at least `\0` as well. And your mangling will collide strings if they contain `\1`. I think you should probably just have this use internal linkage (and a meaningless symbol name) for strings containing bad characters. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:1081 + ObjCStrGV->setSection(ConstantStringSection); + ObjCStrGV->setLinkage(llvm::GlobalValue::ExternalLinkage); + ObjCStrGV->setComdat(TheModule.getOrInsertComdat(StringName)); ---------------- Surely this has to be `linkonce_odr`? ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:1097 + ASTContext &Context = CGM.getContext(); + Fields.add(MakeConstantString(property->getNameAsString())); + std::string TypeStr = ---------------- Field declaration comments, like you use elsewhere, would help here. And something that includes the struct name that you're generating as well. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:1126 + auto MethodList = Builder.beginStruct(); + // int count; + MethodList.addInt(IntTy, Methods.size()); ---------------- Same comment here about adding a comment somewhere that mentions the struct name from the runtime header that you're generating. ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:1196 + case Qualifiers::OCL_None: + default: + Flag = 0; ---------------- Please make this exhaustive, since you're just missing `Qualifiers::OCL_Autoreleasing` (which is illegal on an ivar, of course). ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:1216 + llvm::GlobalValue::ExternalLinkage, nullptr, Name); + GV->setAlignment(4); + } ---------------- Why 4? ================ Comment at: lib/CodeGen/CGObjCGNU.cpp:1395 + auto *SelStart = GetSectionStart(SelSection); + auto *SelEnd = GetSectionStop(SelSection); + auto *ClsStart = GetSectionStart(ClsSection); ---------------- Maybe you can have a method that returns the start/end symbols as a pair? It would cut down on the redundancy here a lot. Repository: rC Clang https://reviews.llvm.org/D46052 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits