[PATCH] D76977: [lld-macho] Implement basic export trie

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/MachO/ExportTrie.cpp:47
+
+  StringRef subString;
+  struct TrieNode *child;
----------------
super nit: substring is a single word


================
Comment at: lld/MachO/ExportTrie.cpp:75
+
+void TrieNode::addSymbol(const Symbol &entry,
+                         std::vector<TrieNode *> &allNodes) {
----------------
ruiu wrote:
> int3 wrote:
> > ruiu wrote:
> > > `addSymbol` seem to keep the internal trie consistent all the time, but I don't think we need to split the work between `addSymbol` and `build`. We can make `addSymbol` just to store an argument to an array, and move the code for trie construction to `build`.
> > Not sure I understand what you're going for. Do you mean to have TrieBuilder::addSymbol collect the symbols to export in a vector, and then have `build()` call `TrieNode::addSymbol()` for all the symbols right before serializing the trie?
> Yes, that's what I meant.
What I wanted to say is that it looks like the trie implemented in this file is designed with the generic Map use cases in mind. You can add a new string one at a time to an existing trie, and you can also look it up as if it were a map (it is actually a map). In order to that, we create a trie in memory. But I think we can avoid that. We can just accumulate added strings to a vector, and in build() we sort the strings and directly emit a serialized trie from the sorted strings.


================
Comment at: lld/MachO/ExportTrie.h:23
+  void addSymbol(const Symbol &sym);
+  llvm::SmallVector<char, 128> build();
+
----------------
int3 wrote:
> ruiu wrote:
> > I don't think we need to be efficient here. I'd use std::vector<> instead.
> There doesn't seem to be a way to wrap an std::vector in a `raw_ostream` though, which I need for the ULEB encoding functions. Would you prefer I use an `std::string` and a `raw_string_ostream` instead?
Yeah, I guess so. Symbol tables are not that small, and there's only one symbol table for each output, so we don't need to optimize it by using an LLVM-specific class. `std::string` would just work fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76977