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

[Will Dietz]

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 --------------
A non-text attachment was scrubbed...
Name: 0001-ASTVector-Fix-return-value-of-various-insert-methods.patch
Type: text/x-patch
Size: 4627 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140821/69d6e1b7/attachment.bin>