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

[Richard Smith]

+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
> >>
> >>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140824/62f10b93/attachment.html>