[PATCH] ASTVector: Fix return value of various insert() methods

[Will Dietz]

Thank you, good call.

Added mentioned test and committed as r216385.

Thanks!

~Will

On Mon, Aug 25, 2014 at 12:24 AM, Richard Smith <richard at metafoo.co.uk> wrote:
> +TEST_F(ASTVectorTest, InsertFill) {
> +  ASTVector<double> V;
> +
> +  // Ensure returned iterator points to first of inserted elements
> +  auto I = V.insert(Ctxt, V.begin(), 5, 1.0);
> +  ASSERT_EQ(I, V.begin());
> +
> +  // Check non-empty case as well
> +  auto J = V.insert(Ctxt, V.begin() + 1, 5, 1.0);
> +  ASSERT_EQ(J, V.begin() + 1);
>  }
>
> You don't have any tests for the insert-at-the-end, non-empty case for this
> form of 'insert', as far as I can see. Looks like the test would still pass
> if I did this:
>
>    iterator insert(const ASTContext &C, iterator I, size_type NumToInsert,
>                    const T &Elt) {
>      // Convert iterator to elt# to avoid invalidating iterator when we
> reserve()
>      size_t InsertElt = I - this->begin();
>
>      if (I == this->end()) { // Important special case for empty vector.
>        append(C, NumToInsert, Elt);
> -      return this->begin() + InsertElt;
> +      return this->begin();
>      }
>
> LGTM with one more test case for the above.
>
>
> On Thu, Aug 21, 2014 at 4:17 PM, Will Dietz <wdietz2 at illinois.edu> wrote:
>>
>> Hi Richard, @cfe-commits,
>>
>> Attached is an updated patch, including actual tests for ASTVector.
>>
>> There's still a long way to go to verify other aspects of ASTVector's
>> behavior as a well-behaved container, but this covers the functionality
>> changes made in this commit and should serve as a basis for more thorough
>> testing in the future should someone tackle such a task :).
>>
>> Please let me know okay to commit or if there's any questions or comments
>> :).
>>
>> Thanks!
>>
>> ~Will
>>
>>
>> On Tue, Nov 19, 2013 at 9:18 AM, Will Dietz <wdietz2 at illinois.edu> wrote:
>> > Closest we have is a test to ensure ASTVector compiles correctly with
>> > basic usage[1] due to complications in creating a fake ASTContext to
>> > construct it with.  I'd be happy to write some functionality tests if
>> > anyone has a good way to construct one for testing purposes, as I'm
>> > not very familiar with the related code.
>> >
>> > Or should I commit this as improving quality regardless?
>> >
>> > ~Will
>> >
>> > [1]
>> > https://github.com/llvm-mirror/clang/blob/f475bf83a45435a211edb4e0ef6ac3481ce7b3fe/unittests/AST/ASTVectorTest.cpp
>> >
>> > On Tue, Nov 19, 2013 at 2:06 AM, Richard Smith <richard at metafoo.co.uk>
>> > wrote:
>> >> Change LGTM. It'd be nice to have test coverage for this that doesn't
>> >> require running a sanitizer. Do we have any direct tests for ASTVector?
>> >>
>> >>
>> >> On Mon, Nov 18, 2013 at 9:35 PM, Will Dietz <wdietz2 at illinois.edu>
>> >> wrote:
>> >>>
>> >>> Ping! :)
>> >>>
>> >>> ~Will
>> >>>
>> >>> On Mon, Nov 4, 2013 at 4:32 PM, Will Dietz <wdietz2 at illinois.edu>
>> >>> wrote:
>> >>> > Ping.
>> >>> >
>> >>> > It's easy to get clang to trigger this bug which results in an
>> >>> > invalid
>> >>> > iterator to be returned (which the current code happens to ignore,
>> >>> > but
>> >>> > that's just a lucky coincidence), as this regularly occurs during
>> >>> > execution of the lit tests.
>> >>> >
>> >>> > On a related note, any suggestions on how to create a simple dummy
>> >>> > ASTContext for testing? As noted in
>> >>> > the commit that originally added ASTVectorTest.cpp (r186253) this
>> >>> > blocks the creation of even basic
>> >>> > functionality tests for this data structure.
>> >>> >
>> >>> > ~Will
>> >>> >
>> >>> > On Mon, Oct 28, 2013 at 5:11 PM, Will Dietz <wdietz2 at illinois.edu>
>> >>> > wrote:
>> >>> >> Error caught -fsanitize=pointer-overflow[1], curiously enough :).
>> >>> >>
>> >>> >> The pointer overflow occurred when insert() was invoked with
>> >>> >> From==To,
>> >>> >> which is done in quite a few places.  While std::vector::insert
>> >>> >> requires [From,To) to be valid, it looks like here From==To is
>> >>> >> intended to be supported[2], making the bug in the container not in
>> >>> >> its use.
>> >>> >>
>> >>> >> This patch fixes the overflow when From==To, as well as the return
>> >>> >> value in this variant as well as the "fill" variant, changing them
>> >>> >> to
>> >>> >> return an iterator pointing to the first of the inserted elements
>> >>> >> (like SmallVector does).
>> >>> >>
>> >>> >> See attached.
>> >>> >>
>> >>> >> ~Will
>> >>> >>
>> >>> >> [1] Patches coming soon.
>> >>> >> [2] See the implementation of append(), for example.
>> >>> _______________________________________________
>> >>> cfe-commits mailing list
>> >>> cfe-commits at cs.uiuc.edu
>> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> >>
>> >>
>
>