[PATCH] D63344: lld/elf: Deduplicate undefined symbol diagnostics

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:718
+  std::vector<Loc> Locs;
+  bool IsWarning;
+};
----------------
thakis wrote:
> grimar wrote:
> > MaskRay wrote:
> > > grimar wrote:
> > > > Seems this field can be calculated at any time, does it worth to keep the precalculated value?
> > > `IsWarning` is computed on a per-symbol basis:
> > > 
> > > ```
> > > bool CanBeExternal = !Sym.isLocal() && Sym.computeBinding() != STB_LOCAL &&
> > >                        Sym.Visibility == STV_DEFAULT;
> > > 
> > >   bool IsWarning =
> > >       (Config->UnresolvedSymbols == UnresolvedPolicy::Warn && CanBeExternal) ||
> > >       Config->NoinhibitExec;
> > > 
> > > ```
> > Yes and we have `Symbol * UndefinedDiag::Sym` here, so can calculate it on fly I think.
> It's needed as a return value; doing the computation twice seems less good to me?
I think usually we do not store things that can be computated trivially. Lets see what others think about this one though.


================
Comment at: lld/ELF/Relocations.cpp:778
+  // collect all "referenced from" lines at the first diagnostic.
+  std::map<Symbol*, size_t> FirstRef;
+  for (size_t i = 0; i < Undefs.size(); ++i) {
----------------
thakis wrote:
> grimar wrote:
> > I'd use llvm::DenseMap. It is probably a bit more consistent with the rest LLD ELF code,
> > and allows then to use its `lookup` method below:
> > 
> > ```
> > if (Symbol *S = FirstRef.lookup(Undef.Sym)) {
> > ...
> > }
> > ```
> Done, I think. (I had to change the value type to UndefinedDiag, and do `if (UndefinedDiag *...` to make it work; that's my best guess at what you meant. Maybe I got it wrong.)
You got it right.


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

https://reviews.llvm.org/D63344