[lld] r210008 - Simplify markLive().

[Nick Kledzik]

On Jun 2, 2014, at 5:57 PM, Rui Ueyama <ruiu at google.com> wrote:

> On Mon, Jun 2, 2014 at 2:02 PM, Nick Kledzik <kledzik at apple.com> wrote:
> 
> On Jun 2, 2014, at 1:06 AM, Rui Ueyama <ruiu at google.com> wrote:
> > Author: ruiu
> > Date: Mon Jun  2 03:06:57 2014
> > New Revision: 210008
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=210008&view=rev
> > Log:
> > Simplify markLive().
> >
> > Reference::target() never returns a nullptr, so NULL check
> > is not needed and is more harmful than doing nothing.
> > No functionality change.
> Reference::target() can return nullptr.  From the header:
> 
>   /// If the reference is an edge to another Atom, then this returns the
>   /// other Atom.  Otherwise, it returns nullptr.
>   virtual const Atom *target() const = 0;
> 
> As the comment for the Reference class explains, they can be used for carrying
> extra data about an atom in which case the target() part is unused.
> 
> Is that comment still relevant? There are other places that assumes target() never returns nullptr, and all uses of References in LLD are to represent edges in graph of Atoms. The name "reference" also implies that it refers something.
> 
> So, if we need an attribute that has no target, I feel like it should be represented by something other than Reference.
I think in a discussion long ago we realized that if an atom wants to use a Reference for internal purposes (such “here is a constant island”), it won’t be using the target part, so it can set the reference’s target to be the atom itself.  Then we never have nullptr targets.

So, the comments should be updated to remove mentioning that the target can be nullptr.

Given that the target cannot be nullptr, does it make sense to change it from Atom* to Atom&  ?

-Nick



> 
> 
> -Nick
> 
> 
> >
> > Modified:
> >    lld/trunk/include/lld/Core/Resolver.h
> >    lld/trunk/lib/Core/Resolver.cpp
> >
> > Modified: lld/trunk/include/lld/Core/Resolver.h
> > URL: http://llvm.org/viewvc/llvm-project/lld/trunk/include/lld/Core/Resolver.h?rev=210008&r1=210007&r2=210008&view=diff
> > ==============================================================================
> > --- lld/trunk/include/lld/Core/Resolver.h (original)
> > +++ lld/trunk/include/lld/Core/Resolver.h Mon Jun  2 03:06:57 2014
> > @@ -67,7 +67,7 @@ private:
> >   void checkDylibSymbolCollisions();
> >   void forEachUndefines(bool searchForOverrides, UndefCallback callback);
> >
> > -  void markLive(const Atom &atom);
> > +  void markLive(const Atom *atom);
> >   void addAtoms(const std::vector<const DefinedAtom *>&);
> >
> >   class MergedFile : public MutableFile {
> >
> > Modified: lld/trunk/lib/Core/Resolver.cpp
> > URL: http://llvm.org/viewvc/llvm-project/lld/trunk/lib/Core/Resolver.cpp?rev=210008&r1=210007&r2=210008&view=diff
> > ==============================================================================
> > --- lld/trunk/lib/Core/Resolver.cpp (original)
> > +++ lld/trunk/lib/Core/Resolver.cpp Mon Jun  2 03:06:57 2014
> > @@ -305,17 +305,16 @@ void Resolver::updateReferences() {
> > }
> >
> > // For dead code stripping, recursively mark atoms "live"
> > -void Resolver::markLive(const Atom &atom) {
> > +void Resolver::markLive(const Atom *atom) {
> >   // Mark the atom is live. If it's already marked live, then stop recursion.
> > -  auto exists = _liveAtoms.insert(&atom);
> > +  auto exists = _liveAtoms.insert(atom);
> >   if (!exists.second)
> >     return;
> >
> >   // Mark all atoms it references as live
> > -  if (const DefinedAtom *defAtom = dyn_cast<DefinedAtom>(&atom))
> > +  if (const DefinedAtom *defAtom = dyn_cast<DefinedAtom>(atom))
> >     for (const Reference *ref : *defAtom)
> > -      if (const Atom *target = ref->target())
> > -        markLive(*target);
> > +      markLive(ref->target());
> > }
> >
> > // remove all atoms not actually used
> > @@ -342,7 +341,7 @@ void Resolver::deadStripOptimize() {
> >
> >   // mark all roots as live, and recursively all atoms they reference
> >   for (const Atom *dsrAtom : _deadStripRoots)
> > -    markLive(*dsrAtom);
> > +    markLive(dsrAtom);
> >
> >   // now remove all non-live atoms from _atoms
> >   _atoms.erase(std::remove_if(_atoms.begin(), _atoms.end(), [&](const Atom *a) {
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at cs.uiuc.edu
> > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> 
> 

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140602/a3ac5f2a/attachment.html>