Skip to content

Conversation

@jayfoad
Copy link
Contributor

@jayfoad jayfoad commented Jan 17, 2025

This code was using a pre-move-semantics trick of using std::swap to
avoid expensive vector copies.

This code was using a pre-move-semantics trick of using std::swap to
avoid expensive vector copies.
@llvmbot
Copy link
Member

llvmbot commented Jan 17, 2025

@llvm/pr-subscribers-testing-tools

Author: Jay Foad (jayfoad)

Changes

This code was using a pre-move-semantics trick of using std::swap to
avoid expensive vector copies.


Full diff: https://github.com/llvm/llvm-project/pull/123304.diff

2 Files Affected:

  • (modified) llvm/lib/FileCheck/FileCheck.cpp (+4-4)
  • (modified) llvm/lib/FileCheck/FileCheckImpl.h (+3-2)
diff --git a/llvm/lib/FileCheck/FileCheck.cpp b/llvm/lib/FileCheck/FileCheck.cpp
index a6df9672f81008..f7faa3ada3a200 100644
--- a/llvm/lib/FileCheck/FileCheck.cpp
+++ b/llvm/lib/FileCheck/FileCheck.cpp
@@ -1933,8 +1933,8 @@ bool FileCheck::readCheckFile(
     }
 
     // Okay, add the string we captured to the output vector and move on.
-    CheckStrings.emplace_back(P, UsedPrefix, PatternLoc);
-    std::swap(DagNotMatches, CheckStrings.back().DagNotStrings);
+    CheckStrings.emplace_back(P, UsedPrefix, PatternLoc,
+                              std::move(DagNotMatches));
     DagNotMatches = ImplicitNegativeChecks;
   }
 
@@ -1963,8 +1963,8 @@ bool FileCheck::readCheckFile(
   if (!DagNotMatches.empty()) {
     CheckStrings.emplace_back(
         Pattern(Check::CheckEOF, PatternContext.get(), LineNumber + 1),
-        *Req.CheckPrefixes.begin(), SMLoc::getFromPointer(Buffer.data()));
-    std::swap(DagNotMatches, CheckStrings.back().DagNotStrings);
+        *Req.CheckPrefixes.begin(), SMLoc::getFromPointer(Buffer.data()),
+        std::move(DagNotMatches));
   }
 
   return false;
diff --git a/llvm/lib/FileCheck/FileCheckImpl.h b/llvm/lib/FileCheck/FileCheckImpl.h
index c772eddd8ecd5e..b94ab544476917 100644
--- a/llvm/lib/FileCheck/FileCheckImpl.h
+++ b/llvm/lib/FileCheck/FileCheckImpl.h
@@ -837,8 +837,9 @@ struct FileCheckString {
   /// Hold the DAG/NOT strings occurring in the input file.
   std::vector<DagNotPrefixInfo> DagNotStrings;
 
-  FileCheckString(const Pattern &P, StringRef S, SMLoc L)
-      : Pat(P), Prefix(S), Loc(L) {}
+  FileCheckString(const Pattern &P, StringRef S, SMLoc L,
+                  std::vector<DagNotPrefixInfo> &&D)
+      : Pat(P), Prefix(S), Loc(L), DagNotStrings(std::move(D)) {}
 
   /// Matches check string and its "not strings" and/or "dag strings".
   size_t Check(const SourceMgr &SM, StringRef Buffer, bool IsLabelScanMode,

@jayfoad
Copy link
Contributor Author

jayfoad commented Jan 17, 2025

I'm not the world's biggest C++ expert so I'd appreciate someone checking my work here.

Copy link
Collaborator

@jh7370 jh7370 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming the pre-merge checks pass.

Comment on lines 840 to 842
FileCheckString(const Pattern &P, StringRef S, SMLoc L,
std::vector<DagNotPrefixInfo> &&D)
: Pat(P), Prefix(S), Loc(L), DagNotStrings(std::move(D)) {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you are at it, you might consider switching const Pattern &P to the move semantics. Pattern looks big, involving std::vector, std::map, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Done.

@jayfoad jayfoad merged commit d3d605b into llvm:main Jan 23, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants