[llvm-commits] [llvm] r160854 - /llvm/trunk/include/llvm/ADT/SmallVector.h

[Benjamin Kramer]

On 27.07.2012, at 11:14, Chandler Carruth <chandlerc at google.com> wrote:

> On Fri, Jul 27, 2012 at 2:10 AM, Benjamin Kramer <benny.kra at googlemail.com> wrote:
> Author: d0k
> Date: Fri Jul 27 04:10:25 2012
> New Revision: 160854
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=160854&view=rev
> Log:
> SmallVector::erase: Assert that iterators are actually inside the vector.
> 
> The rationale here is that it's hard to write loops containing vector erases and
> it only shows up if the vector contains non-trivial objects leading to crashes
> when forming them out of garbage memory.
> 
> Modified:
>     llvm/trunk/include/llvm/ADT/SmallVector.h
> 
> Modified: llvm/trunk/include/llvm/ADT/SmallVector.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallVector.h?rev=160854&r1=160853&r2=160854&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/SmallVector.h (original)
> +++ llvm/trunk/include/llvm/ADT/SmallVector.h Fri Jul 27 04:10:25 2012
> @@ -463,6 +463,7 @@
>    }
> 
>    iterator erase(iterator I) {
> +    assert(I >= this->begin() && I < this->end() && "Iterator out of bounds");
> 
> What do you think about making these separate asserts so that the line number (and message) tell you exactly what went wrong instead of just that something went wrong? It's a bit more code, but not that much...

Good idea, added more verbose asserts in r160882.

> Also, is it worth factoring these asserts into a generic pair of functions, one to validate a single iterator, one to validate a begin/end pair, and use them more consistently throughout APIs that accept such iterators?

While it would be great to add asserts to other methods, I think making it a separate method is overkill here. It would also kill the advantage we gained by splitting up the assertions, the line numbers would be merged again.

The only other candidate in SmallVector that needs iterator validation I could think of is insert, also fixed in r160882.

- Ben
> 
>      iterator N = I;
>      // Shift all elts down one.
>      this->move(I+1, this->end(), I);
> @@ -472,6 +473,8 @@
>    }
> 
>    iterator erase(iterator S, iterator E) {
> +    assert(S >= this->begin() && S <= E && E <= this->end() &&
> +           "Iterator range out of bounds");
>      iterator N = S;
>      // Shift all elts down.
>      iterator I = this->move(E, this->end(), S);
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>