[NFC] Replace uses of find(x) != end() idiom with contains(x) for StringRef and Set-like types #35229

mininny · 2020-12-27T12:49:49Z

A little while back, LLVM Set-like data types got contains(t) methods which make code more readable. Many places in the compiler use the find(a) != .end() idiom to check for membership (and in some places find(a) == .end() to check if the item is not present).

It would be nice to update the code to use contains(a) to improve the readability.

This PR replaces usages of .find(Key) != .end() idiom with .contains(Key).

Resolves SR-13950.

@varungandhi-apple Could you please review this? :) 🙏

mininny · 2020-12-27T18:19:47Z

unittests/runtime/Mutex.cpp

+          } else {
+            if (trace)
+              printf("Worker[%d] HIT for key = %d, value = %d.\n", i, key,
+                     value->second);
+            found = true; // cache hit, no need to grab write lock
          }
-          if (trace)
-            printf("Worker[%d] HIT for key = %d, value = %d.\n", i, key,
-                   value->second);
-          found = true; // cache hit, no need to grab write lock


I also added the missing else in this code, but I'm not sure if it is okay to fix it in this PR as well?

I would put it in a different PR. Different high level thing.

gottesmm

A few thoughts:

You are not only changing set like things here. In fact, it makes me worried in general about this patch. Instead you are changing some sort like things and in some cases StringRef. Please separate the StringRef changes into a separate commit.
I found at least 1-2 cases where the patch is changing a single lookup into multiple lookups. Please remove that (I marked it).

Before this goes in I want to look at it again.

lib/Driver/Driver.cpp

lib/Frontend/DiagnosticVerifier.cpp

lib/LLVMPasses/ARCEntryPointBuilder.h

lib/SILOptimizer/Analysis/ARCAnalysis.cpp

gottesmm · 2020-12-29T21:03:28Z

lib/SILOptimizer/PassManager/PassManager.cpp

@@ -233,8 +230,7 @@ static void printModule(SILModule *Mod, bool EmitVerboseSIL) {
        std::find(SILPrintOnlyFun.begin(), SILPrintOnlyFun.end(), F.getName()))
      F.dump(EmitVerboseSIL);

-    if (!SILPrintOnlyFuns.empty() &&
-        F.getName().find(SILPrintOnlyFuns, 0) != StringRef::npos)


A note for myself. Whats with the 0? (I am doing a surface level review scan now).

@mininny I looked in StringRef.h. Looks like the 0 is a default argument to find that says start checking at the beginning. (Just answering my own question).

gottesmm · 2020-12-29T21:09:15Z

Just please move that last commit to a different PR and I think this is ready to go in.

gottesmm · 2020-12-29T21:09:32Z

@swift-ci test

gottesmm · 2020-12-29T21:10:04Z

Doing a quick test to see if everything works.

swift-ci · 2020-12-29T21:25:10Z

Build failed
Swift Test OS X Platform
Git Sha - 0af551930dafd9c1a36b9f2cd524eed860b37e55

swift-ci · 2020-12-29T21:53:08Z

Build failed
Swift Test Linux Platform
Git Sha - 0af551930dafd9c1a36b9f2cd524eed860b37e55

LucianoPAlmeida · 2020-12-30T00:36:35Z

lib/LLVMPasses/LLVMARCOpts.cpp

@@ -142,8 +142,8 @@ static bool canonicalizeInputFunction(Function &F, ARCEntryPointBuilder &B,
        // Have not encountered a strong retain/release. keep it in the
        // unknown retain/release list for now. It might get replaced
        // later.
-        if (NativeRefs.find(ArgVal) == NativeRefs.end()) {
-           UnknownObjectRetains[ArgVal].push_back(&CI);
+        if (!NativeRefs.contains(ArgVal)) {


Seems the build is failing becausellvm::DenseSet doesn't have a contains method, @gottesmm we normally use count so perhaps we could use this here as well? :)

It seems like the change to llvm::DenseSet hasn't landed on swift/main branch so it's causing this issue..? The contains method exists on llvm-project's apple/stable/20210107 branch; I'm not sure how the CI works but can this be fixed by moving the base branch? :)

Yeah, seems like it hasn't ... I would say that use count instead for Set like would just be fine and already an improvement, but since the whole point of the SR is to use contains I'm not really sure about that, so cc @varungandhi-apple @gottesmm

Hmm, I seem to have made a mistake here. I looked at the git log and I saw that contains was added for SmallPtrSet, so I figured methods for all the Set-like data types were available on swift/main, since they were merged in succession to LLVM's main branch. However, looks like it's only present for SmallPtrSet on swift/main. Sorry about that.

We can wait for a couple of weeks; I'll ping this PR when we have the rebranch so we can run tests and land the change. I don't think cherry-picking changes onto other branches is the right fix here, as that might lead to more merge conflicts.

CodaFi · 2021-01-05T19:51:43Z

LGTM

@swift-ci test

swift-ci · 2021-01-05T20:36:22Z

Build failed
Swift Test Linux Platform
Git Sha - 09faa98bd81cd1338489071610262e5c158d3070

swift-ci · 2021-01-05T21:52:00Z

Build failed
Swift Test OS X Platform
Git Sha - 09faa98bd81cd1338489071610262e5c158d3070

CodaFi · 2021-01-05T23:54:07Z

Please revert the DenseSet refactoring

/home/buildnode/jenkins/workspace/swift-PR-Linux/branch-main/swift/lib/LLVMPasses/LLVMARCOpts.cpp:145:25: error: no member named 'contains' in 'llvm::DenseSet<llvm::Value *, llvm::DenseMapInfo<llvm::Value *> >'
14:36:13         if (!NativeRefs.contains(ArgVal)) {
14:36:13              ~~~~~~~~~~ ^
14:36:13 /home/buildnode/jenkins/workspace/swift-PR-Linux/branch-main/swift/lib/LLVMPasses/LLVMARCOpts.cpp:192:25: error: no member named 'contains' in 'llvm::DenseSet<llvm::Value *, llvm::DenseMapInfo<llvm::Value *> >'
14:36:13         if (!NativeRefs.contains(ArgVal)) {
14:36:13              ~~~~~~~~~~ ^

varungandhi-apple · 2021-01-06T15:53:38Z

@CodaFi I posted this in a thread

We can wait for a couple of weeks; I'll ping this PR when we have the rebranch so we can run tests and land the change. I don't think cherry-picking changes onto other branches is the right fix here, as that might lead to more merge conflicts.

Reverting is going to be more work, instead we can wait it out for changes to land on the LLVM side.

varungandhi-apple · 2021-01-20T19:32:40Z

Mishal just posted information on the rebranch in the forums: https://forums.swift.org/t/updating-llvm-project-swift-main-branch/44019. We can land this change once the rebranch is done.

varungandhi-apple · 2021-02-02T18:50:35Z

@mininny can you resolve the merge conflicts? I can run the tests after that.

varungandhi-apple · 2021-02-03T01:25:03Z

I think it's okay for now, but for the future, please prefer to rebase for resolving conflicts instead of merging, so that we have a cleaner history.

@swift-ci smoke test

mininny · 2021-02-03T01:27:06Z

I think it's okay for now, but for the future, please prefer to rebase for resolving conflicts instead of merging, so that we have a cleaner history.

@swift-ci smoke test

Oh didn't realize that! Okay, Thanks! :)

….contains(Key)

varungandhi-apple · 2021-02-05T16:21:57Z

@swift-ci please smoke test

lib/Frontend/DiagnosticVerifier.cpp

LucianoPAlmeida · 2021-02-06T10:16:56Z

@swift-ci smoke test

varungandhi-apple · 2021-02-06T16:35:44Z

@swift-ci smoke test macOS

mininny commented Dec 27, 2020

View reviewed changes

gottesmm reviewed Dec 28, 2020

View reviewed changes

mininny force-pushed the switch-find-to-contains branch 3 times, most recently from 65e65fa to 0af5519 Compare December 28, 2020 15:31

gottesmm changed the title ~~[NFC] Replace uses of find(x) != end() idiom with contains(x) for Set-like types~~ [NFC] Replace uses of find(x) != end() idiom with contains(x) for StringRef and Set-like types Dec 29, 2020

gottesmm reviewed Dec 29, 2020

View reviewed changes

gottesmm approved these changes Dec 29, 2020

View reviewed changes

LucianoPAlmeida reviewed Dec 30, 2020

View reviewed changes

mininny force-pushed the switch-find-to-contains branch from 0af5519 to 09faa98 Compare December 30, 2020 01:21

Replace usages of StringRef.find(Key) != StringRef::npos to StringRef…

e924cf6

….contains(Key)

varungandhi-apple reviewed Feb 6, 2021

View reviewed changes

lib/Frontend/DiagnosticVerifier.cpp Outdated Show resolved Hide resolved

mininny force-pushed the switch-find-to-contains branch from c215f4d to 2d8df59 Compare February 6, 2021 09:22

Replace usages of .find(Key) != .end() to .contains(Key) in llvm sets

53d1fc4

mininny force-pushed the switch-find-to-contains branch from 2d8df59 to 53d1fc4 Compare February 6, 2021 09:24

varungandhi-apple merged commit caf1a55 into swiftlang:main Feb 8, 2021

typesanitizer mentioned this pull request Feb 8, 2021

[SR-13950] Replace uses of find(x) != end() idiom with contains(x) for Set-like types #56347

Closed

[NFC] Replace uses of find(x) != end() idiom with contains(x) for StringRef and Set-like types #35229

[NFC] Replace uses of find(x) != end() idiom with contains(x) for StringRef and Set-like types #35229

Uh oh!

Conversation

mininny commented Dec 27, 2020

Uh oh!

mininny Dec 27, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm Dec 29, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gottesmm Dec 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gottesmm Dec 29, 2020

Choose a reason for hiding this comment

Uh oh!

gottesmm commented Dec 29, 2020

Uh oh!

gottesmm commented Dec 29, 2020

Uh oh!

gottesmm commented Dec 29, 2020

Uh oh!

swift-ci commented Dec 29, 2020

Uh oh!

swift-ci commented Dec 29, 2020

Uh oh!

LucianoPAlmeida Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mininny Dec 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucianoPAlmeida Dec 30, 2020

Choose a reason for hiding this comment

Uh oh!

varungandhi-apple Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Jan 5, 2021

Uh oh!

swift-ci commented Jan 5, 2021

Uh oh!

swift-ci commented Jan 5, 2021

Uh oh!

CodaFi commented Jan 5, 2021

Uh oh!

varungandhi-apple commented Jan 6, 2021

Uh oh!

varungandhi-apple commented Jan 20, 2021

Uh oh!

varungandhi-apple commented Feb 2, 2021

Uh oh!

varungandhi-apple commented Feb 3, 2021

Uh oh!

mininny commented Feb 3, 2021

Uh oh!

varungandhi-apple commented Feb 5, 2021

Uh oh!

Uh oh!

LucianoPAlmeida commented Feb 6, 2021

Uh oh!

varungandhi-apple commented Feb 6, 2021

Uh oh!

Uh oh!

gottesmm Dec 29, 2020 •

edited

Loading

LucianoPAlmeida Dec 30, 2020 •

edited

Loading

mininny Dec 30, 2020 •

edited

Loading