[llvm] r221958 - StringMap: Test and finish off supporting perfectly forwarded values in StringMap operations.
David Blaikie
dblaikie at gmail.com
Tue Nov 18 10:14:22 PST 2014
On Mon, Nov 17, 2014 at 3:17 PM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:
>
> > On 2014-Nov-17, at 11:26, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Thu, Nov 13, 2014 at 4:51 PM, Duncan P. N. Exon Smith <
> dexonsmith at apple.com> wrote:
> >
> > > On 2014-Nov-13, at 16:41, David Blaikie <dblaikie at gmail.com> wrote:
> > >
> > > Author: dblaikie
> > > Date: Thu Nov 13 18:41:46 2014
> > > New Revision: 221958
> > >
> > > URL: http://llvm.org/viewvc/llvm-project?rev=221958&view=rev
> > > Log:
> > > StringMap: Test and finish off supporting perfectly forwarded values
> in StringMap operations.
> > >
> > > Followup to r221946.
> >
> > I'm not convinced this is necessary when there are in-tree users,
> > but I suppose it can't hurt anything.
> >
> > Easier to iterate when someone goes to refactor it & ensures the
> codepaths continue to be tested even if a certain use case later gets
> refactored to not use that feature anymore.
> >
> >
> > >
> > > Modified:
> > > llvm/trunk/include/llvm/ADT/StringMap.h
> > > llvm/trunk/unittests/ADT/StringMapTest.cpp
> > >
> > > Modified: llvm/trunk/include/llvm/ADT/StringMap.h
> > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/StringMap.h?rev=221958&r1=221957&r2=221958&view=diff
> > >
> ==============================================================================
> > > --- llvm/trunk/include/llvm/ADT/StringMap.h (original)
> > > +++ llvm/trunk/include/llvm/ADT/StringMap.h Thu Nov 13 18:41:46 2014
> > > @@ -170,9 +170,9 @@ public:
> > >
> > > /// Create - Create a StringMapEntry with normal malloc/free.
> > > template<typename InitType>
> > > - static StringMapEntry *Create(StringRef Key, InitType InitVal) {
> > > + static StringMapEntry *Create(StringRef Key, InitType &&InitVal) {
> > > MallocAllocator A;
> > > - return Create(Key, A, std::move(InitVal));
> > > + return Create(Key, A, std::forward<InitType>(InitVal));
> > > }
> >
> > This hardly adds new functionality. The lvalue version collides with the
> > `Create(StringRef, AllocatorTy&)` overload.
> >
> > Which no one is using? (or I would've just made the code fail to
> compile? depending on the exact nature of the collision) Again, why I love
> having tests - if it's required functionality, I should've broken something.
> >
>
> Right, but you've extended functionality from my prior commit, and
> added tests for what you're guessing my purpose was. It's a rather
> awkward way for us to co-develop ;).
>
> I still think a test that "this API will compile" isn't useful. It
> permanently adds time to everyone's build, and doesn't catch any
> regressions (at least, none that aren't caught by other in-tree
> users failing to compile).
>
Only if those uses remain in-tree, which isn't always the case. Bitrotting
behavior because the testing is lost or incomplete isn't great.
I agree that "does this compile" tests are of limited value (as much here
as they are in clang) without testing for particular behavior - which could
be added. Though adding compile-time because we're testing something
doesn't seem like an argument not to test - the same tradeoff is made for
any testing at all (& if we really want to fix that, we'd need some kind of
dependency based test execution so we don't run tests for unchanged code).
> It would have saved us both time if I hadn't broken the commit up
> into two, so perhaps I should just collapse them in the future when
> I think a unit test is harmful.
>
I'd still argue that the ADT should be tested is isolation - for the same
reason we have all the existing ADT tests. It ensures they're well tested
and tested even when we refactor existing code in such a way as to not use
a certain piece of functionality for a time. (ADT functionality tests are
obviously really high value - testing corner cases, etc, is much easier to
debug in a unit test than when an optimization doesn't fire correctly
because a container's misbehaving)
>
> > This will *only* add
> > functionality for a temporary being passed in. (See my comment below.)
> >
> > A workaround would be to invert the `StringRef` and `AllocatorTy` args,
> > but really you should always be passing the map's allocator.
> >
> > Really? Lots of them use the default, so far as I can see.
> >
> > Maybe this overload should be deleted entirely?
> >
> > Maybe - but it seems it's used more than the other.
> >
>
> I must have had a `grep`-fail :(. It looked to me like there were
> almost no users of this, most users went through the `StringMap`.
>
Hmm, nope - could've been my mistake - I don't really see any uses of
StringMapEntry<>::Create - which seems good to me. I just meant that since
I was able to introduce that ambiguity without breaking any builds there
mustn't be any uses of that functionality. (& I thought I'd seen a variety
of uses of raw StringMapEntry::Create before - but maybe I was just
recalling the unit tests)
Which is good - raw uses of StringMapEntry shouldn't really be common (it's
sort of weird that we expose that in the API at all, to be honest).
>
> >
> > >
> > > static StringMapEntry *Create(StringRef Key) {
> > > @@ -367,8 +367,9 @@ public:
> > > /// exists, return it. Otherwise, default construct a value, insert
> it, and
> > > /// return.
> > > template <typename InitTy>
> > > - MapEntryTy &GetOrCreateValue(StringRef Key, InitTy Val) {
> > > - return *insert(std::make_pair(Key, std::move(Val))).first;
> > > + MapEntryTy &GetOrCreateValue(StringRef Key, InitTy &&Val) {
> > > + return *insert(std::pair<StringRef, ValueTy>(
> > > + Key, std::forward<InitTy>(Val))).first;
> > > }
> > >
> > > MapEntryTy &GetOrCreateValue(StringRef Key) {
> > >
> > > Modified: llvm/trunk/unittests/ADT/StringMapTest.cpp
> > > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/StringMapTest.cpp?rev=221958&r1=221957&r2=221958&view=diff
> > >
> ==============================================================================
> > > --- llvm/trunk/unittests/ADT/StringMapTest.cpp (original)
> > > +++ llvm/trunk/unittests/ADT/StringMapTest.cpp Thu Nov 13 18:41:46 2014
> > > @@ -256,9 +256,15 @@ TEST_F(StringMapTest, NonDefaultConstruc
> > > ASSERT_EQ(iter->second.i, 123);
> > > }
> > >
> > > +struct Immovable {
> > > + Immovable() {}
> > > + Immovable(Immovable&&) LLVM_DELETED_FUNCTION; // will disable the
> other special members
> > > +};
> > > +
> > > struct MoveOnly {
> > > int i;
> > > MoveOnly(int i) : i(i) {}
> > > + MoveOnly(const Immovable&) : i(0) {}
> > > MoveOnly(MoveOnly &&RHS) : i(RHS.i) {}
> > > MoveOnly &operator=(MoveOnly &&RHS) {
> > > i = RHS.i;
> > > @@ -270,7 +276,7 @@ private:
> > > MoveOnly &operator=(const MoveOnly &) LLVM_DELETED_FUNCTION;
> > > };
> > >
> > > -TEST_F(StringMapTest, MoveOnlyKey) {
> > > +TEST_F(StringMapTest, MoveOnly) {
> > > StringMap<MoveOnly> t;
> > > t.GetOrCreateValue("Test", MoveOnly(42));
> > > StringRef Key = "Test";
> > > @@ -278,6 +284,14 @@ TEST_F(StringMapTest, MoveOnlyKey) {
> > > ->Destroy();
> > > }
> > >
> > > +TEST_F(StringMapTest, CtorArg) {
> > > + StringMap<MoveOnly> t;
> > > + t.GetOrCreateValue("Test", Immovable());
> > > + StringRef Key = "Test";
> > > + StringMapEntry<MoveOnly>::Create(Key, Immovable())
> > > + ->Destroy();
> >
> > In particular, this test would fail:
> >
> > Immovable I;
> > StringMapEntry<MoveOnly>::Create(Key, I)
> >
> > Agreed.
> >
> > Though on further reflection, I'm not sure this whole line of changes is
> the right direction - will discuss more on your use-case commit
> >
> >
> > > +}
> > > +
> > > TEST_F(StringMapTest, MoveConstruct) {
> > > StringMap<int> A;
> > > A.GetOrCreateValue("x", 42);
> > >
> > >
> > > _______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at cs.uiuc.edu
> > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141118/4e7baef5/attachment.html>
More information about the llvm-commits
mailing list