llvm dev - Mar 2011 - [LLVMdev] [Patch] Fix bug in llvm::SmallVectorIml<>::insert

This fixes a bug in SmallVectorImpl<>::insert, which were not behaving 
correctly on inserting an empty range into an empty vector:

#include <llvm/ADT/SmallVector.h>
#include <cassert>

int main() {
  llvm::SmallVector<int, 1> v, w;
  llvm::SmallVector<int, 1>::iterator it     v.insert(v.end(), w.begin(),
w.end());
  assert(it == v.end());
}

The insert function(s) would incorrectly return "this->end()-1". I
attached
the patch which I diff'ed from trunk. Is it important enough to be 
backported to llvm2.9 ?

I would like to ask someone to commit it to wherever it fits. Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: fix_smallvector.patch
Type: text/x-patch
Size: 906 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20110319/9943fcd2/attachment.bin>

Johannes Schaub (litb) wrote:
> This fixes a bug in SmallVectorImpl<>::insert, which were not
behaving
> correctly on inserting an empty range into an empty vector:
> 
> #include <llvm/ADT/SmallVector.h>
> #include <cassert>
> 
> int main() {
>   llvm::SmallVector<int, 1> v, w;
>   llvm::SmallVector<int, 1>::iterator it >     v.insert(v.end(),
w.begin(), w.end());
>   assert(it == v.end());
> }
> 
> The insert function(s) would incorrectly return
"this->end()-1". I
> attached the patch which I diff'ed from trunk. Is it important enough
to
> be backported to llvm2.9 ?
> 
> I would like to ask someone to commit it to wherever it fits. Thanks!
Hmm, I don't understand the rationale of the return value of the range-
insert and repeated-insert.

The range-insert returns an iterator pointing exactly at the last value 
inserted. 

But the repeated-insert (i.e Insert(Position, RepeatCount, Value)) 
apparently returns an iterator pointing to the first value inserted. Is this 
actually intended?

My patch is inconsistent with this in mind, because it always returns a 
pointer to the first value inserted. Please don't apply it yet. We need to 
clear this up first and fix my patch.

On Sat, Mar 19, 2011 at 2:26 PM, Johannes Schaub (litb)
<schaub.johannes at googlemail.com> wrote:
> Johannes Schaub (litb) wrote:
>
>> This fixes a bug in SmallVectorImpl<>::insert, which were not
behaving
>> correctly on inserting an empty range into an empty vector:
>>
>> #include <llvm/ADT/SmallVector.h>
>> #include <cassert>
>>
>> int main() {
>>   llvm::SmallVector<int, 1> v, w;
>>   llvm::SmallVector<int, 1>::iterator it >>    
v.insert(v.end(), w.begin(), w.end());
>>   assert(it == v.end());
>> }
>>
>> The insert function(s) would incorrectly return
"this->end()-1". I
>> attached the patch which I diff'ed from trunk. Is it important
enough to
>> be backported to llvm2.9 ?
>>
>> I would like to ask someone to commit it to wherever it fits. Thanks!
>
> Hmm, I don't understand the rationale of the return value of the range-
> insert and repeated-insert.
>
> The range-insert returns an iterator pointing exactly at the last value
> inserted.
>
> But the repeated-insert (i.e Insert(Position, RepeatCount, Value))
> apparently returns an iterator pointing to the first value inserted. Is
this
> actually intended?
>
> My patch is inconsistent with this in mind, because it always returns a
> pointer to the first value inserted. Please don't apply it yet. We need
to
> clear this up first and fix my patch.
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
Well, assuming that SmallVector should be as similar to std::vector as
possible, and looking at n3290 23.2.3/{8,9}:

8 The iterator returned from a.insert(p, n, t) points to the copy of
the first element inserted into a, or p if n == 0.
9 The iterator returned from a.insert(p, i, j) points to the copy of
the first element inserted into a, or p if i == j.

The answer is rather clear.

- Michael Spencer