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