[PATCH] D63344: lld/elf: Deduplicate undefined symbol diagnostics
George Rimar via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 18 05:14:03 PDT 2019
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
More information about the llvm-commits
mailing list