This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Enable set_difference() to be used on StringSet
ClosedPublic

Authored by mmpozulp on May 24 2019, 12:34 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

mmpozulp created this revision.May 24 2019, 12:34 AM

This patch must land before https://reviews.llvm.org/D62275 in which I use set_difference() on StringSet

I'm not sure I know enough about LLVM's ADT classes to be able to properly review this (on the surface the code looks fine though), so I've added a few reviewers who have worked on some of the *Set classes in the past according to the history. If you don't hear back from any of them by the end of next week, ping me and I'll take a look.

For my part, this seems reasonable to me.

dblaikie added inline comments.May 24 2019, 9:10 AM
llvm/unittests/ADT/SetOperationsTest.cpp
1–29

This test probably should go where other StringSet tests are (presumably unittests/ADT/StringSetTest) - and I would /generally/ be in favor of not 'integration testing' that set_difference works with StringSet, but that StringSet exposes the appropriate kind of iterators/operations that set_difference requires in general?

Oh, I see, this llvm::set_difference doesn't just wrap std::set_difference, so it has different interface requirements. I'd /probably/ still test the surface area of StringSet directly - and certainly feel like code changes to StringSet would be tested in StringSet's tests.

mmpozulp updated this revision to Diff 201342.May 24 2019, 2:51 PM

Incorporated feedback from @dblaikie to change test name from SetOperationsTest.cpp to StringSetTest.cpp and moved an existing StringSet test from StringMapTest.cpp to new file StringSetTest.cpp.

dblaikie accepted this revision.May 26 2019, 12:56 PM
dblaikie added inline comments.
llvm/unittests/ADT/StringSetTest.cpp
33–42 ↗(On Diff #201342)

I'd still prefer this as two unit tests for the two new functions you added (& the test wouldn't use set_difference at all - unit tests being what they are, try to test things in isolation) - but I'll leave it up to you. No big deal (:

This revision is now accepted and ready to land.May 26 2019, 12:56 PM
mmpozulp updated this revision to Diff 202059.May 29 2019, 2:40 PM

Incorporate feedback from @dblaikie to call insert(StringMapEntry) and count(StringMapEntry) instead of set_difference(StringSet, StringSet) in StringSetTest.cpp, which better adheres to the spirit of unit-testing.

Can you commit this yourself - or would you like me to do it for you?

mmpozulp updated this revision to Diff 202066.May 29 2019, 2:48 PM

Change a const char * to a StringRef

Can you commit this yourself - or would you like me to do it for you?

Yes, please commit for me. I just requested commit access by email to Chris on Friday. No response yet so I assume I don't have access, which is fine because helpful people like you can do it for me while I wait. Thanks! :)

@dblaikie, did this get committed? I think @mmpozulp you may now have access so could commit if it hasn't happened yet.

@dblaikie, did this get committed? I think @mmpozulp you may now have access so could commit if it hasn't happened yet.

Yeah, had my local client tangled up with some other work for a bit so I haven't got to committing this - but if @mmpozulp has commit access now - then, yeah, he should go ahead and commit it so the author information's more accurate, etc.

I'm fighting with svn. I ran svn co https://user@llvm.org/svn/llvm-project/llvm/trunk llvm which took about 90 minutes to checkout the repo. If I run svn up it hangs for ~10 minutes on a poll() call (which I discovered by running strace) then emits an error

$ svn up
Updating '.':
svn: E175002: REPORT of '/svn/llvm-project/!svn/vcc/default': Could not read status line: Secure connection truncated (https://llvm.org)

I'm running svn 1.7.14

$ svn --version
svn, version 1.7.14 (r1542130)

Good news! Apparently I can commit from git (https://llvm.org/docs/GettingStarted.html#for-developers-to-commit-changes-from-git). Thank you, @smeenai!!! I should be able to land this commit soon. Good-bye svn :)

mmpozulp updated this revision to Diff 203482.Jun 6 2019, 8:09 PM

Delete unused header in StringSetTest.cpp.

mmpozulp closed this revision.Jun 6 2019, 8:37 PM
mmpozulp reopened this revision.Jun 6 2019, 8:39 PM
This revision is now accepted and ready to land.Jun 6 2019, 8:39 PM
mmpozulp closed this revision.EditedJun 6 2019, 8:41 PM

I committed as r362766. Thanks! Note that the Differential Revision listed in the r362766 commit message is D62992 instead of D62369. That was my mistake caused by an accidental arc diff instead of arc diff --update. I posted a link in D62992 to D62369. Sorry for the confusion.

mmpozulp reopened this revision.Jun 7 2019, 12:48 PM
This revision is now accepted and ready to land.Jun 7 2019, 12:48 PM
mmpozulp updated this revision to Diff 203606.Jun 7 2019, 1:06 PM

Fix asan-detected stack-buffer-overflow in StringSetTest.cpp

@dblaikie approved the stack-buffer-overflow fix (see https://reviews.llvm.org/D63000) which I have applied here, so I'm going to re-land this patch :)

mmpozulp edited the summary of this revision. (Show Details)Jun 7 2019, 1:15 PM
This revision was automatically updated to reflect the committed changes.

Committed as r362835.