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

[David Chisnall via Phabricator via cfe-commits]

theraven added inline comments.


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:977
+    if ((CGM.getTarget().getPointerWidth(0) == 64) &&
+        (SL->getLength() < 9) && !isNonASCII) {
+      // Tiny strings are (roughly):
----------------
rjmccall wrote:
> Please hoist `SL->getLength()` into a variable.
> 
> Also, consider breaking this bit of code (maybe just building a `uint64_t`?) into a separate function.
I don't think separating it would simplify the code much.  It's 8 lines of non-comment code and you'd probably need at least three of them in the caller.


================
Comment at: lib/CodeGen/CGObjCGNU.cpp:1077
+    std::replace(StringName.begin(), StringName.end(),
+      '@', '\1');
+    auto *ObjCStrGV =
----------------
rjmccall wrote:
> 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.
No, I copied the mangling for selectors without engaging my brain.  I've now replaced it with something much more conservative.  I might tweak it a bit before the next release, but this seems to give pretty good coverage.


Repository:
  rC Clang

https://reviews.llvm.org/D46052