<br><br>On Friday, July 31, 2015, Aaron Ballman <<a href="mailto:aaron@aaronballman.com">aaron@aaronballman.com</a>> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Wed, Jul 29, 2015 at 8:40 PM, Puyan Lotfi <<a href="javascript:;" onclick="_e(event, 'cvml', 'plotfi@apple.com')">plotfi@apple.com</a>> wrote:<br>
> 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>
Comments below. Btw, I think this discussion should be on its own<br>
thread instead of attached to a commit revert message. I suspect there<br>
are others in the community who have opinions on this ADT, but are<br>
unaware of the discussion due to context. I've taken the liberty of<br>
starting a new email thread for this, and included a few usual<br>
suspects who review ADTs.<br>
<br>
For anyone joining in the middle, this is a discussion of an ADT Puyan<br>
proposed in:<br>
<a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150720/288930.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150720/288930.html</a><br>
<br>
That was commit (and eventually reverted) in:<br>
<a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150727/289945.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20150727/289945.html</a><br>
<br>
> diff --git a/include/llvm/ADT/SortedVector.h b/include/llvm/ADT/SortedVector.h<br>
> new file mode 100644<br>
> index 0000000..ddafd6b<br>
> --- /dev/null<br>
> +++ b/include/llvm/ADT/SortedVector.h<br>
> @@ -0,0 +1,137 @@<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>
> +<br>
> +namespace llvm {<br>
> +<br>
> +/// \brief A vector with minimal append overhead that enforces a sorted<br>
> +/// ordering of unique elements prior to access. The most common use case<br>
> +/// is to push_back multiple Entries and call sortUnique afterward.<br>
<br>
I am wondering why this should be an ADT instead of one-off<br>
functionality. I see that you're going to use it for<br>
MachineBasicBlock, but are there other use cases as well? The kind of<br>
functionality that this class provides is covered by basic STL<br>
mechanisms, and so I am hesitant to support it as an ADT for all of<br>
our users unless this is something we expect will be heavily used (or<br>
is heavily customized to support toolchain needs).<br>
<br>
Also, the comments do not specify what ordering it is sorted by (we<br>
can presume from the std::less, but it would be good to be explicit<br>
since this is user-facing documentation).<br>
<br>
> +template<typename T, typename CMP = std::less<T>><br>
> +class SortedVector {<br>
> +private:<br>
> + typedef T value_type;<br>
> + typedef typename std::vector<value_type> vector_type;<br>
> + typedef typename vector_type::const_iterator const_iterator;<br>
> + typedef typename vector_type::iterator iterator;<br>
> + typedef typename vector_type::size_type size_type;<br>
<br>
These typedefs should be public, aside from vector_type.<br>
<br>
Missing typedefs for difference_type, reference and const_reference.<br>
<br>
> +<br>
> + vector_type Vector;<br>
> + bool IsSorted = true;<br>
> +<br>
> + void AssertIsSorted() const {<br>
> + assert(IsSorted && "Unsorted SortedVector access; call sortUnique prior.");<br>
> + }<br>
> +<br>
> +public:<br>
> + /// \brief Appends Entry to the end of Vector; avoids appending duplicate<br>
> + /// entries. If appending Entry causes Vector to be unsorted, then sortUnique<br>
> + /// must be called prior to access.<br>
<br>
The public comments are still discussing private implementation<br>
details like Vector.<br>
<br>
How is the user of this function supposed to know when appending the<br>
entry causes the vector to be unsorted?<br>
<br>
> + void push_back(const value_type &Entry) {<br>
<br>
Should be using the typedef for const_reference.<br>
<br>
> + if (!Vector.size())<br>
<br>
Vector.empty() is more clear.<br>
<br>
> + Vector.push_back(Entry);<br>
<br>
After pushing back in an empty vector, why are you relying on the<br>
std::equal_to to perform the return?<br>
<br>
> +<br>
> + // If Entry is a duplicate of the previous Entry we skip.<br>
> + if (std::equal_to<value_type>()(Vector.back(), Entry))<br>
> + return;<br>
<br>
Why is this forcing use of std::equal_to, but we can specialize on<br>
something other than std::less? If someone is implementing a custom<br>
std::less implementation, it's very likely they would want a custom<br>
std::equal_to used as well.<br>
<br>
> +<br>
> + IsSorted &= (CMP()(Vector.back(), Entry));<br>
> + Vector.push_back(Entry);<br>
> + }<br>
> +<br>
> + // \brief Sorts and uniques Vector.<br>
> + void sortUnique() {<br>
> + if (IsSorted)<br>
> + return;<br>
<br>
This still isn't correct. Consider code like:<br>
<br>
SortedVector<int> V;<br>
V.push_back(1);<br>
V.push_back(2);<br>
V.push_back(3);<br>
*V.begin() = 3;<br>
<br>
Not only is the vector no longer sorted (but it will claim that it is<br>
because IsSorted will be true), it's also not unique. We then fail to<br>
sort or unique the backing store.<br>
<br>
In order for this ADT to be viable, the underlying vector elements<br>
must be const so that you can never mutate their values through the<br>
ADT interface.<br>
<br>
> +<br>
> + std::sort(Vector.begin(), Vector.end());<br>
> + Vector.erase(std::unique(Vector.begin(), Vector.end()), Vector.end());<br>
<br>
Neither of these STL functions are using the custom comparator.<br>
Instead they use operator< on the object type, which is inconsistent.<br>
Should pass in CMP() to std::sort(), and should pass in the template<br>
parameter for std::equal_to for std::unique().<br>
<br>
> + IsSorted = true;<br>
> + }<br>
> +<br>
> + /// \brief Tells if Entry is in Vector without relying on sorted-uniqueness.<br>
> + bool has(value_type Entry) const {<br>
<br>
This should be using const_reference instead of forcing a by-value copy.<br>
<br>
> + if (IsSorted)<br>
> + return std::binary_search(Vector.begin(), Vector.end(), Entry);<br>
<br>
This function is using operator< to perform the comparisons instead of<br>
using CMP().<br>
<br>
> +<br>
> + return std::find(Vector.begin(), Vector.end(), Entry) != Vector.end();<br>
<br>
This function is using operator== to perform the comparisons instead<br>
of using a template parameter for std::equal_to.<br>
<br>
> + }<br>
<br>
I find it really strange that so many member functions call<br>
AssertIsSorted(), but the only truly interesting part of the interface<br>
does not. Why is it valid for you to call has() without using a sorted<br>
vector? This means the complexity of calling this function is either<br>
O(N) or O(log N), and you have no way of knowing which you'll get<br>
unless you call sortUnique() first.<br>
<br>
> +<br>
> + /// \brief Returns a reference to the Entry with the specified index.<br>
> + const value_type &operator[](unsigned index) const {<br>
<br>
This should be returning a const_reference and taking a size_type.<br>
<br>
> + assert(index < size() && "SortedVector index is out of range!");<br>
> + AssertIsSorted();<br>
> + return Vector[index];<br>
> + }<br>
> +<br>
> + /// \brief returns an iterator to the start of vector. Asserts if vector is<br>
> + /// not sorted and uniqued.<br>
> + iterator begin() {<br>
> + AssertIsSorted();<br>
> + return Vector.begin();<br>
> + }<br>
> +<br>
> + /// \brief Returns a const_iterator to the start of Vector. Asserts if<br>
> + /// Vector is not sorted and uniqued.<br>
> + const_iterator begin() const {<br>
> + AssertIsSorted();<br>
> + return Vector.begin();<br>
> + }<br>
> +<br>
> + /// \brief returns an iterator to the end of vector. Asserts if vector is<br>
> + /// not sorted and uniqued.<br>
> + iterator end() {<br>
> + AssertIsSorted();<br>
> + return Vector.end();<br>
> + }<br>
> +<br>
> + /// \brief Returns a const_iterator to the end of Vector. Asserts if<br>
> + /// Vector is not sorted and uniqued.<br>
> + const_iterator end() const {<br>
> + AssertIsSorted();<br>
> + return Vector.end();<br>
> + }<br>
> +<br>
> + /// \brief Erases Vector at position. Asserts if Vector not sorted and<br>
> + /// uniqued.<br>
> + iterator erase(iterator position) {<br>
> + AssertIsSorted();<br>
<br>
A vector iterator is a direct index into the vector; why is this<br>
assertion required for erasing an arbitrary element?<br>
<br>
> + return Vector.erase(position);<br>
> + }<br>
> +<br>
> + /// \brief Returns the total number of entries in Vector. Asserts if Vector<br>
> + /// not sorted and uniqued.<br>
> + size_type size() const {<br>
> + AssertIsSorted();<br>
<br>
Why is this assertion required for testing the size?<br>
<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 Entries in Vector.<br>
> + void clear() {<br>
> + IsSorted = true;<br>
> + Vector.clear();<br>
> + }<br>
> +};<br>
<br>
There is a lot of functionality missing from this interface that are<br>
concerns. For instance, there's no rvalue references, no constructors,<br>
pop_back, rbegin()/rend(), etc. This seems like a very limited<br>
interface in some ways, while way too permissive of an interface in<br>
other ways.<br>
<br>
Even if you expanded the interface to cover more of the useful cases<br>
and fix the issues from the review, I am not certain this meets the<br>
bar to be an ADT. This, ultimately, is the stock STL vector that needs<br>
to remain ordered and unique. Why is an existing ordered, unique list<br>
(like std::set) insufficient?<br>
<br></blockquote><div><br></div><div>I forgot to mention that this ADT also avoids having to sort the vector in many of the common cases where entries are appended to Machine BasicBlock::Lives by maintaining whether the vector is sorted at every push_back. The vector is normally already sorted but not always. It keeps compile time impact lower by doing this.<span></span></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
~Aaron<br>
<br>
> +<br>
> +} // End of namespace llvm<br>
> +<br>
> +#endif // LLVM_ADT_SORTEDVECTOR_H<br>
> +<br>
><br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="javascript:;" onclick="_e(event, 'cvml', 'llvm-commits@cs.uiuc.edu')">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote>