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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lld/ELF/Relocations.cpp:718
+  std::vector<Loc> Locs;
+  bool IsWarning;
+};
----------------
grimar wrote:
> 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.
Repeating the same code in two functions may be bad, but if you factor the code out and define a predicate function to determine whether we should report an error or a warning, it might look good. It may be worth trying.


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

https://reviews.llvm.org/D63344