This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Add contains(KeyType) -> bool methods to Set types.
ClosedPublic

Authored by varungandhi-apple on Jul 8 2020, 7:32 PM.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJul 8 2020, 7:32 PM

I don't understand the clang-tidy/clang-format complaints. I've formatted the code based on how the surrounding code is formatted.

fhahn added a subscriber: fhahn.

Nice, IMO this makes a lot of sense and will help to get rid of a lot of more verbose find(x) != end() expressions.

For most ADT classes, there are unit tests in llvm/unittests/ADT/. It would be good to add additional tests there. Also ,it would be good to add a proper doc-comment in all classes, as well as including the prior art comment in the description and replacing [llvm] with [ADT].

I don't understand the clang-tidy/clang-format complaints. I've formatted the code based on how the surrounding code is formatted.

IIRC, these come from arcanist. They should behave themselves if you have this utility on your PATH: https://github.com/llvm/llvm-project/blob/master/clang/tools/clang-format/git-clang-format#L15

+1 needs unit tests, might make sense to commit each one separately with its test (& maybe some cleanup commits afterwards, to help migrate the codebase/encourage future use in the new direction)

Added comments and test cases addressing fhahn's review comment.

I've added some unit tests. Are these adequate/do you have any other suggestions on what I should test?

might make sense to commit each one separately with its test

I'm not sure what benefit this would bring. Could you elaborate? I would've understood if the combined change were large by itself, but that's not the case here.

maybe some cleanup commits afterwards, to help migrate the codebase/encourage future use in the new direction

I could do this in a separate revision, if desired.

dblaikie accepted this revision.Jul 15 2020, 12:24 PM

I've added some unit tests. Are these adequate/do you have any other suggestions on what I should test?

Looks good to me, thanks!

might make sense to commit each one separately with its test

I'm not sure what benefit this would bring. Could you elaborate? I would've understood if the combined change were large by itself, but that's not the case here.

The usual sentiment that independent changes should be submitted independently - means if something goes wrong they can be reverted in isolation without reverting unrelated changes (eg: if there was a bug in one of these implementations - it got submitted, you committed a few more changes on top of it to start using these APIs more broadly - then someone identified/reported the bug, we rolled back the patch... now we'd have to rollback all the cleanups too - rather than only those cleanups that used the particular API that was buggy & leaving the rest intact)

maybe some cleanup commits afterwards, to help migrate the codebase/encourage future use in the new direction

I could do this in a separate revision, if desired.

I think that'd be nice, thanks!

This revision is now accepted and ready to land.Jul 15 2020, 12:24 PM
fhahn accepted this revision.Jul 15 2020, 1:30 PM

LGTM as well, thanks! You could commit this as separate patches for each container.

varungandhi-apple updated this revision to Diff 278364.EditedJul 15 2020, 8:38 PM

Split up single commit into several commits (one commit per set type).

@dblaikie could you land the patch for me? I do not have bot permissions/commit privileges.

This revision was automatically updated to reflect the committed changes.

Thanks! It seems a bit weird that this revision now only shows the one commit under Revision contents even though the 'Commits' above shows the six commits. 🤷

dblaikie added a comment.EditedJul 17 2020, 12:04 PM

Thanks! It seems a bit weird that this revision now only shows the one commit under Revision contents even though the 'Commits' above shows the six commits. 🤷

Yeah, I doubt there's much to be done about that. Imagine phab only has the concept of a review being closed by one commit.

Oh, and sorry I forgot to update the "author" field to appropriately credit you when I committed these :/ they all have the Phab link in them at least, which leads here for appropriate credit.

Oh, and sorry I forgot to update the "author" field to appropriately credit you when I committed these

Nbd, thanks for the review and merging. :)