[PATCH] D98223: [lld-macho] implement options -(un)exported_symbol(s_list)

[Greg McGary via Phabricator via llvm-commits]

gkm marked 4 inline comments as done.
gkm added inline comments.


================
Comment at: lld/MachO/Config.h:53-54
+class SymbolPatterns {
+  llvm::DenseSet<llvm::StringRef> literals;
+  std::vector<llvm::GlobPattern> globs;
+
----------------
int3 wrote:
> Hmm, GlobPattern.cpp has a special case for patterns without wildcards, but I guess having a separate literals set is still faster with the O(1) lookup
> 
> Since we're optimizing for speed here, I would use `CachedHashStringRef` for the literals set
Yes, the separate literals set is for performance. I will comment it as such.


================
Comment at: lld/MachO/Driver.cpp:741
+bool SymbolPatterns::matchGlob(StringRef symbolName) const {
+  for (auto const &glob : globs)
+    if (glob.match(symbolName))
----------------
int3 wrote:
> avoid `auto`
11 of 37 range-for loops in `lld/MachO/Driver.cpp` use `auto`. I will cleanup with a separate [NFC] diff.


================
Comment at: lld/test/MachO/export-options.s:16-20
+.macro DEFSYM, type, sym
+\type \sym
+\sym:
+  retq
+.endm
----------------
int3 wrote:
> I'm not sure I see much value in these macros... you're not really saving that many characters in exchange for an additional level of indirection
> 
> Without macros, you could save on the `retq`s by doing
> 
> ```
> _keep_globl:
> _hide_globl:
> _keep_private:
> _show_private:
>   retq
> ```
> 
> just my 2c, keep the macros if you like them
My primary goals were to (a) minimize vertical space, and (b) eliminate the need to type the symbol name twice--once for the scope directive, and again for the label. Placing all of the labels at the same address doesn't help me with the latter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98223