[PATCH] D46052: GNUstep Objective-C ABI version 2

[John McCall via Phabricator via cfe-commits]

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