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

Wed Jul 15 12:24:23 PDT 2020

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

In D83449#2154087 <https://reviews.llvm.org/D83449#2154087>, @varungandhi-apple wrote:

> 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!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83449/new/

https://reviews.llvm.org/D83449