Page MenuHomePhabricator

ADT: Fix reference invalidation in N-element SmallVector::append and insert
ClosedPublic

Authored by dexonsmith on Wed, Dec 23, 2:02 PM.

Details

Summary

For small enough, trivially copyable T, take the parameter by-value in
SmallVector::append and SmallVector::insert. Otherwise, when
growing, update the argument appropriately.

Depends on https://reviews.llvm.org/D93779.
Split out from https://reviews.llvm.org/D91837.

Diff Detail

Event Timeline

dexonsmith created this revision.Wed, Dec 23, 2:02 PM
dexonsmith requested review of this revision.Wed, Dec 23, 2:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Dec 23, 2:02 PM
dblaikie accepted this revision.Wed, Dec 23, 3:47 PM
dblaikie added inline comments.
llvm/unittests/ADT/SmallVectorTest.cpp
1156

Might be worth making this V.capacity() - V.size() + 1 to make it more precise? While V.capacity() will always be enough, I think (to me at least) that might make the test a bit harder to read, since capacity() may be significantly more than needed.

This revision is now accepted and ready to land.Wed, Dec 23, 3:47 PM
This revision was landed with ongoing or failed builds.Wed, Jan 13, 8:20 PM
This revision was automatically updated to reflect the committed changes.
dexonsmith marked an inline comment as done.Wed, Jan 13, 8:25 PM

Thanks for the review! Landed in 3043e5a5c33c4c871f4a1dfd621a8839f9a1f0b3.

llvm/unittests/ADT/SmallVectorTest.cpp
1156

Good point. I updated as suggested in the commit.

In the similar case for the InsertN test below, I decided to expand the comment and leave it as V.capacity() since being precise there made it harder to reason about (since there we care about NumToInsert being larger than something).