<div dir="ltr">Ping</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Jul 29, 2015 at 5:40 PM, Puyan Lotfi <span dir="ltr"><<a href="mailto:plotfi@apple.com" target="_blank">plotfi@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Justin, Aaron:<br>
<br>
These are the revised patches.<br>
I have tried to address all of your feedback with the exception of the naming of the ADT.<br>
I was thinking, if I did rename it I could go with SortedVectorSet or SortedSet.<br>
<br>
Anyways, I have added:<br>
<br>
- Better documentation changes that try to be more clear on how and when to use SortedVector.<br>
- Better comments.<br>
- Removed erase().<br>
- Replaced reset with clear.<br>
- Renamed insert to push_back<br>
- Added unit testing.<br>
<br>
Hope this is enough to put my patch back in; please let me know if I missed anything.<br>
<br>
Thanks<br>
<span class="HOEnZb"><font color="#888888"><br>
PL<br>
<br>
</font></span><br><br>
<br>
<br>
<br>
> On Jul 27, 2015, at 11:50 PM, Justin Bogner <<a href="mailto:mail@justinbogner.com">mail@justinbogner.com</a>> wrote:<br>
><br>
> Puyan Lotfi <<a href="mailto:puyan@puyan.org">puyan@puyan.org</a>> writes:<br>
>> Author: zer0<br>
>> Date: Tue Jul 28 01:04:00 2015<br>
>> New Revision: 243386<br>
>><br>
>> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D243386-26view-3Drev&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=rKQbKkAS6reW2mUEAtLioZIbRJT3_ictGCGE0laWpPQ&s=0kDdhi-n8Di7KEISJ6qIKrbaAWqyRPSlznDkiOfFUvQ&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=243386&view=rev</a><br>
>> Log:<br>
>> Adding ADT SortedVector; client patch will follow.<br>
>><br>
>> Added:<br>
>> llvm/trunk/include/llvm/ADT/SortedVector.h<br>
>> Modified:<br>
>> llvm/trunk/docs/ProgrammersManual.rst<br>
>><br>
>> Modified: llvm/trunk/docs/ProgrammersManual.rst<br>
>> URL:<br>
>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_docs_ProgrammersManual.rst-3Frev-3D243386-26r1-3D243385-26r2-3D243386-26view-3Ddiff&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=rKQbKkAS6reW2mUEAtLioZIbRJT3_ictGCGE0laWpPQ&s=uNhtLlx_6l9ETE1BOJ1zzsxwiI4fCwrl2ThC98alvNI&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/docs/ProgrammersManual.rst?rev=243386&r1=243385&r2=243386&view=diff</a><br>
>> ==============================================================================<br>
>> --- llvm/trunk/docs/ProgrammersManual.rst (original)<br>
>> +++ llvm/trunk/docs/ProgrammersManual.rst Tue Jul 28 01:04:00 2015<br>
>> @@ -1314,7 +1314,8 @@ never use hash_set and unordered_set bec<br>
>> std::multiset is useful if you're not interested in elimination of duplicates,<br>
>> but has all the drawbacks of :ref:`std::set <dss_set>`. A sorted vector<br>
>> (where you don't delete duplicate entries) or some other approach is almost<br>
>> -always better.<br>
>> +always better. LLVM actually offers SortedVector which does the job of a sorted<br>
>> +std::vector.<br>
><br>
> Please elaborate a bit here. This doesn't really explain why you would<br>
> choose SortedVector over a sorted std::vector. Similarly, sorted vectors<br>
> aren't necessarily uniq'd, so it isn't really the same job.<br>
><br>
>><br>
>> .. _ds_map:<br>
>><br>
>><br>
>> Added: llvm/trunk/include/llvm/ADT/SortedVector.h<br>
>> URL: <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_include_llvm_ADT_SortedVector.h-3Frev-3D243386-26view-3Dauto&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=rKQbKkAS6reW2mUEAtLioZIbRJT3_ictGCGE0laWpPQ&s=2ihfNKGTQ6L_ojVZdbdZ1ihl39WC21pBA154Pu1SsSg&e=" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SortedVector.h?rev=243386&view=auto</a><br>
>> ==============================================================================<br>
>> --- llvm/trunk/include/llvm/ADT/SortedVector.h (added)<br>
>> +++ llvm/trunk/include/llvm/ADT/SortedVector.h Tue Jul 28 01:04:00 2015<br>
>> @@ -0,0 +1,133 @@<br>
>> +//===-- llvm/ADT/SortedVector.h ---------------------------------*- C++ -*-===//<br>
>> +//<br>
>> +// The LLVM Compiler Infrastructure<br>
>> +//<br>
>> +// This file is distributed under the University of Illinois Open Source<br>
>> +// License. See LICENSE.TXT for details.<br>
>> +//<br>
>> +//===----------------------------------------------------------------------===//<br>
>> +<br>
>> +#ifndef LLVM_ADT_SORTEDVECTOR_H<br>
>> +#define LLVM_ADT_SORTEDVECTOR_H<br>
>> +<br>
>> +#include <vector><br>
>> +#include <cassert><br>
>> +#include <functional><br>
>> +#include "llvm/Support/raw_ostream.h"<br>
>> +<br>
>> +namespace llvm {<br>
>> +<br>
>> +/// \brief Lazily maintains a sorted and unique vector of elements of type T.<br>
><br>
> This description is a bit misleading. What this really does is enforce<br>
> (through asserts) that you sort the vector before accessing it. When I<br>
> see "lazily maintains" I get the impression that it will (somehow) do<br>
> the sorting for me at the appropriate time.<br>
><br>
>> +template<typename T, typename CMP = std::less<T>><br>
>> +class SortedVector {<br>
><br>
> It might be a good idea to imply uniqueness in the name - sorted doesn't<br>
> tell the whole story.<br>
><br>
>> +public:<br>
>> + typedef typename std::vector<T> VectorType;<br>
>> + typedef typename VectorType::iterator iterator;<br>
>> + typedef typename VectorType::const_iterator const_iterator;<br>
>> +<br>
>> +private:<br>
>> + VectorType Vector;<br>
>> + bool IsSorted = true;<br>
>> +<br>
>> + void doCheck() const {<br>
><br>
> I'd probably call this assertSorted.<br>
><br>
>> + assert(IsSorted && "Unsorted SortedVector access; call sortUnique prior.");<br>
>> + }<br>
>> +<br>
>> +public:<br>
>> + /// \brief Appends Entry to the sorted unique vector; sets the IsSorted flag<br>
>> + /// to false if appending Entry puts Vector into an unsorted state.<br>
><br>
> That we set the IsSorted flag is an implementation detail, better to<br>
> describe this in terms of the user. Something like "Appends Entry. If<br>
> this puts the vector into an unsorted state, sortUnique must be called<br>
> before any accesses to the vector".<br>
><br>
>> + void insert(const T &Entry) {<br>
>> + if (!Vector.size())<br>
>> + Vector.push_back(Entry);<br>
>> +<br>
>> + // Vector is sorted and Entry is a duplicate of the previous so skip.<br>
>> + if (IsSorted && Entry == Vector.back())<br>
>> + return;<br>
>> +<br>
>> + IsSorted &= (CMP()(Vector.back(), Entry));<br>
><br>
> It seems a little odd to use std::less but to call operator== directly,<br>
> doesn't it?<br>
><br>
>> + Vector.push_back(Entry);<br>
>> + }<br>
>> +<br>
>> + // \brief Sorts and uniques Vector.<br>
>> + void sortUnique() {<br>
>> + if (IsSorted)<br>
>> + return;<br>
>> +<br>
>> + std::sort(Vector.begin(), Vector.end());<br>
>> + Vector.erase(std::unique(Vector.begin(), Vector.end()), Vector.end());<br>
>> + IsSorted = true;<br>
>> + }<br>
>> +<br>
>> + /// \brief Tells if Entry is in Vector without relying on sorted-uniqueness.<br>
>> + bool has(T Entry) const {<br>
>> + if (IsSorted)<br>
>> + return std::binary_search(Vector.begin(), Vector.end(), Entry);<br>
>> +<br>
>> + return std::find(Vector.begin(), Vector.end(), Entry) != Vector.end();<br>
>> + }<br>
>> +<br>
>> + /// \brief Returns a reference to the entry with the specified index.<br>
>> + const T &operator[](unsigned index) const {<br>
>> + assert(index < size() && "SortedVector index is out of range!");<br>
>> + doCheck();<br>
>> + return Vector[index];<br>
>> + }<br>
>> +<br>
>> + /// \brief Return an iterator to the start of the vector.<br>
>> + iterator begin() {<br>
>> + doCheck();<br>
>> + return Vector.begin();<br>
>> + }<br>
>> +<br>
>> + /// \brief Returns const iterator to the start of the vector.<br>
>> + const_iterator begin() const {<br>
>> + doCheck();<br>
>> + return Vector.begin();<br>
>> + }<br>
>> +<br>
>> + /// \brief Returns iterator to the end of Vector.<br>
>> + iterator end() {<br>
>> + doCheck();<br>
>> + return Vector.end();<br>
>> + }<br>
>> +<br>
>> + /// \brief Returns const iterator to the end of Vector. Assert if unsorted.<br>
>> + const_iterator end() const {<br>
>> + doCheck();<br>
>> + return Vector.end();<br>
>> + }<br>
>> +<br>
>> + /// \brief Erases Vector at position. Asserts if Vector is unsorted.<br>
>> + iterator erase(iterator position) {<br>
>> + doCheck();<br>
>> + return Vector.erase(position);<br>
>> + }<br>
>> +<br>
>> + /// \brief Erases Vector entirely.<br>
>> + iterator erase() {<br>
>> + IsSorted = true;<br>
>> + return Vector.erase();<br>
>> + }<br>
>> +<br>
>> + /// \brief Returns number of entries in Vector; asserts if it is unsorted.<br>
>> + size_t size() const {<br>
>> + doCheck();<br>
>> + return Vector.size();<br>
>> + }<br>
>> +<br>
>> + /// \brief Returns true if Vector is empty.<br>
>> + bool empty() const {<br>
>> + return Vector.empty();<br>
>> + }<br>
>> +<br>
>> + /// \brief Clears all the entries.<br>
>> + void reset() {<br>
>> + IsSorted = true;<br>
>> + Vector.resize(0, 0);<br>
>> + }<br>
>> +};<br>
>> +<br>
>> +} // End of namespace llvm<br>
>> +<br>
>> +#endif // LLVM_ADT_SORTEDVECTOR_H<br>
>> +<br>
>><br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br>
<br></blockquote></div><br></div>