[PATCH] D93609: [lld/mac] Implement support for private extern symbols

[Jez Ng via Phabricator via llvm-commits]

int3 added a comment.

looks pretty good!



================
Comment at: lld/MachO/Driver.cpp:528
+                           /*isExternal=*/true,
+                           /*isPrivateExtern=*/common->privateExtern);
   }
----------------
nit: seems unnecessary to have the argument name in a comment when the argument itself is descriptive, but I guess it might look more uniform this way. up to you


================
Comment at: lld/MachO/Symbols.h:28
 struct StringRefZ {
-  StringRefZ(const char *s) : data(s), size(-1) {}
+  StringRefZ(const char *s) : StringRefZ(StringRef(s)) {}
   StringRefZ(StringRef s) : data(s.data()), size(s.size()) {}
----------------
This defeats the purpose of using `StringRefZ` over `StringRef` though -- it's meant to be a performance optimization (see the comment on the identically-named class in lld/ELF... I probably should have copied it over and/or factored the class out)

Would a `constexpr` constructor work here?


================
Comment at: lld/MachO/Symbols.h:114
   bool overridesWeakDef : 1;
+  bool privateExtern : 1;
 
----------------
how about combining `privateExtern` and `external` into one enum, so that the invalid state of `external = false, privateExtern = true` is not representable?


================
Comment at: lld/test/MachO/private-extern.s:117
+# RUN: llvm-nm -m %t/csclp | FileCheck --check-prefix=COMMONNONEXTERNAL %s
+# COMMONNONEXTERNAL: (__DATA,__common) non-external (was a private external) _foo
+
----------------
how do you feel about hyphenating the check prefix?


================
Comment at: lld/test/MachO/symtab.s:39
 # CHECK-NEXT:   Symbol {
-# CHECK-NEXT:     Name: _external_weak (65)
+# CHECK-NEXT:     Name: _external (70)
 # CHECK-NEXT:     Extern
----------------
I think including the string offsets in the expected test output was a mistake (probably mine), let's remove it to reduce future churn


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93609/new/

https://reviews.llvm.org/D93609