Thank you, good call. Added mentioned test and committed as r216385.
Thanks! ~Will On Mon, Aug 25, 2014 at 12:24 AM, Richard Smith <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]> >> > 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 <[email protected]> >> >> wrote: >> >>> >> >>> Ping! :) >> >>> >> >>> ~Will >> >>> >> >>> On Mon, Nov 4, 2013 at 4:32 PM, Will Dietz <[email protected]> >> >>> 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 <[email protected]> >> >>> > 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 >> >>> [email protected] >> >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >> >> >> >> > > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
