[llvm] r195241 - [PM] Add the preservation system to the new pass manager.

[Chandler Carruth]

Trying to dig back through stale review that I missed... Sorry for missing
them.

On Fri, May 9, 2014 at 12:15 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

> > +    for (SmallPtrSet<void *, 2>::const_iterator I =
> PreservedPassIDs.begin(),
> > +                                                E =
> PreservedPassIDs.end();
> > +         I != E; ++I)
> > +      if (!Arg.PreservedPassIDs.count(*I))
> > +        PreservedPassIDs.erase(*I);
>
> Interesting that SmallPtrSet doesn't let you erase by iterator.  Is
> that an API oversight, or is it intentional?
>

I have no idea. I'm happy to take a stab at improving this if you want, but
it didn't seem all that bad here. However, I'm not sure I like furthering
the "erase doesn't invalidate iterators too much" property of how we do
hash tables. =/ That has always seemed like a very fragile guarantee. If we
ever move away from it, being able to erase the iterator doesn't help
nearly as much (if at all).


>
> > +  /// \brief Query whether a pass is marked as preserved by this set.
> > +  template <typename PassT> bool preserved() const {
> > +    return preserved(PassT::ID());
> > +  }
> > +
> > +  /// \brief Query whether an abstract pass ID is marked as preserved
> by this
> > +  /// set.
> > +  bool preserved(void *PassID) const {
> > +    return PreservedPassIDs.count((void *)AllPassesID) ||
> > +           PreservedPassIDs.count(PassID);
> > +  }
>
> Shouldn't these functions be spelled "isPreserved()"?
>

They both read fine to me, and so I picked the shorter. In particular,
there isn't the ambiguity of "empty" the verb vs "empty" the state. But if
folks would rather just use 'is' everywhere, I can live with that too.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150107/e0279b59/attachment.html>