<div dir="ltr">Trying to dig back through stale review that I missed... Sorry for missing them.<br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 9, 2014 at 12:15 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">> +    for (SmallPtrSet<void *, 2>::const_iterator I = PreservedPassIDs.begin(),<br>
> +                                                E = PreservedPassIDs.end();<br>
> +         I != E; ++I)<br>
> +      if (!Arg.PreservedPassIDs.count(*I))<br>
> +        PreservedPassIDs.erase(*I);<br>
<br>
</div></div>Interesting that SmallPtrSet doesn't let you erase by iterator.  Is<br>
that an API oversight, or is it intentional?<br></blockquote><div><br></div><div>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).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div><div class="h5"><br>
> +  /// \brief Query whether a pass is marked as preserved by this set.<br>
> +  template <typename PassT> bool preserved() const {<br>
> +    return preserved(PassT::ID());<br>
> +  }<br>
> +<br>
> +  /// \brief Query whether an abstract pass ID is marked as preserved by this<br>
> +  /// set.<br>
> +  bool preserved(void *PassID) const {<br>
> +    return PreservedPassIDs.count((void *)AllPassesID) ||<br>
> +           PreservedPassIDs.count(PassID);<br>
> +  }<br>
<br>
</div></div>Shouldn't these functions be spelled "isPreserved()"?<br></blockquote><div><br></div><div>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.</div></div></div></div>