[PATCH] D62381: Use SymbolTable::insert() to implement --trace.

[Fangrui Song via Phabricator via llvm-commits]

MaskRay accepted this revision.
MaskRay added inline comments.


================
Comment at: lld/ELF/SymbolTable.h:38
 
-  ArrayRef<Symbol *> getSymbols() const { return SymVector; }
+  void forEachSymbol(std::function<void(Symbol *)> Fn) {
+    for (Symbol *Sym : SymVector)
----------------
ruiu wrote:
> MaskRay wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > ruiu wrote:
> > > > > grimar wrote:
> > > > > > `llvm::function_ref` should probably be a bit better, btw.
> > > > > I didn't take a look at the assembly, but isn't this something that a compiler can optimize and erases std::function? Since this is an inline function, a compiler knows exactly how a given lambda is used.
> > > > I do not know the answer.
> > > `function_ref` should be better, but I don't know if compilers can optimize out the heap allocation used for the lambda.
> > > 
> > > I still want to understand how complicated the signature of `llvm::make_filter_range` is. Let me check
> > 
> > ```
> > struct FilterOutPlaceholder {
> >   bool operator()(Symbol *S) const {
> >     return !S->isPlaceholder();
> >   }
> > };
> > 
> > static iterator_range<filter_iterator<std::vector<Symbol *>::iterator, FilterOutPlaceholder>> symbols() {
> >   return make_filter_range(Symtab->SymVector, FilterOutPlaceholder());
> > }
> > ```
> > 
> > The benefit is that the backtrace will be simpler in a debugger.
> We can do that too, but it seems a bit more complicated than the lambda.
> 
> I'm also making a change to split SymVector and SymMap into multiple shards for parallel name resolution. With that, the iterator would have more complicated type, as you need to concatenate multiple iterators and then filter out some members after that.
Since you plan to split SymVector, a linear for look no longer works in call sites, this looks good to me, otherwise I would still think the `for (auto xxx : SymTable->symbols()) ...` is better than `Symtab->forEachSymbol([](Symbol *Sym) { ... });`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D62381