Skip to content

[llvm][ADT] Use ADL to find begin()/end() in interleave* #87669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Apr 5, 2024

Conversation

jroelofs
Copy link
Contributor

@jroelofs jroelofs commented Apr 4, 2024

No description provided.

@llvmbot
Copy link
Member

llvmbot commented Apr 4, 2024

@llvm/pr-subscribers-llvm-adt

Author: Jon Roelofs (jroelofs)

Changes

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

3 Files Affected:

  • (modified) llvm/include/llvm/ADT/STLExtras.h (+2-2)
  • (modified) llvm/unittests/ADT/CMakeLists.txt (+1)
  • (added) llvm/unittests/ADT/Interleave.cpp (+37)
diff --git a/llvm/include/llvm/ADT/STLExtras.h b/llvm/include/llvm/ADT/STLExtras.h
index 02a3074ae1f0d7..b5eb319ccf346e 100644
--- a/llvm/include/llvm/ADT/STLExtras.h
+++ b/llvm/include/llvm/ADT/STLExtras.h
@@ -2151,7 +2151,7 @@ template <typename Container, typename UnaryFunctor, typename NullaryFunctor,
               !std::is_constructible<StringRef, NullaryFunctor>::value>>
 inline void interleave(const Container &c, UnaryFunctor each_fn,
                        NullaryFunctor between_fn) {
-  interleave(c.begin(), c.end(), each_fn, between_fn);
+  interleave(adl_begin(c), adl_end(c), each_fn, between_fn);
 }
 
 /// Overload of interleave for the common case of string separator.
@@ -2159,7 +2159,7 @@ template <typename Container, typename UnaryFunctor, typename StreamT,
           typename T = detail::ValueOfRange<Container>>
 inline void interleave(const Container &c, StreamT &os, UnaryFunctor each_fn,
                        const StringRef &separator) {
-  interleave(c.begin(), c.end(), each_fn, [&] { os << separator; });
+  interleave(adl_begin(c), adl_end(c), each_fn, [&] { os << separator; });
 }
 template <typename Container, typename StreamT,
           typename T = detail::ValueOfRange<Container>>
diff --git a/llvm/unittests/ADT/CMakeLists.txt b/llvm/unittests/ADT/CMakeLists.txt
index 12d7325036bf05..17c5c9d1c59cec 100644
--- a/llvm/unittests/ADT/CMakeLists.txt
+++ b/llvm/unittests/ADT/CMakeLists.txt
@@ -44,6 +44,7 @@ add_llvm_unittest(ADTTests
   ImmutableMapTest.cpp
   ImmutableSetTest.cpp
   IntEqClassesTest.cpp
+  Interleave.cpp
   IntervalMapTest.cpp
   IntervalTreeTest.cpp
   IntrusiveRefCntPtrTest.cpp
diff --git a/llvm/unittests/ADT/Interleave.cpp b/llvm/unittests/ADT/Interleave.cpp
new file mode 100644
index 00000000000000..a5ec19d35e0526
--- /dev/null
+++ b/llvm/unittests/ADT/Interleave.cpp
@@ -0,0 +1,37 @@
+//===- unittests/ADT/IListTest.cpp - ilist unit tests ---------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+TEST(InterleaveTest, Interleave) {
+  std::string Str;
+  raw_string_ostream OS(Str);
+
+  // Check that interleave works on a SmallVector.
+  SmallVector<const char *> Doodles = {"golden", "berna", "labra"};
+  interleave(
+      Doodles, OS, [&](const char *Name) { OS << Name << "doodle"; }, ", ");
+
+  EXPECT_EQ(OS.str(), "goldendoodle, bernadoodle, labradoodle");
+}
+
+TEST(InterleaveTest, InterleaveComma) {
+  std::string Str;
+  raw_string_ostream OS(Str);
+
+  // Check that interleaveComma uses ADL to find begin/end on an array.
+  const StringRef LongDogs[] = {"dachshund", "doxie", "dackel", "teckel"};
+  interleaveComma(LongDogs, OS);
+
+  EXPECT_EQ(OS.str(), "dachshund, doxie, dackel, teckel");
+}

Copy link
Member

@kuhar kuhar left a comment

Choose a reason for hiding this comment

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

LGTM % nits

Copy link

github-actions bot commented Apr 4, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

Copy link

github-actions bot commented Apr 4, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff fb2a380b5d4e483602f8cf8f36ca1df322a14a77 57af3d2e1da231f632f29e6a1d99b864d981cde8 -- llvm/unittests/ADT/Interleave.cpp llvm/include/llvm/ADT/STLExtras.h
View the diff from clang-format here.
diff --git a/llvm/unittests/ADT/Interleave.cpp b/llvm/unittests/ADT/Interleave.cpp
index 64c2659269..20de7ecb09 100644
--- a/llvm/unittests/ADT/Interleave.cpp
+++ b/llvm/unittests/ADT/Interleave.cpp
@@ -1,4 +1,5 @@
-//===- unittests/ADT/Interleave.cpp - Interleave unit tests ----------------===//
+//===- unittests/ADT/Interleave.cpp - Interleave unit tests
+//----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
@@ -6,8 +7,8 @@
 //
 //===----------------------------------------------------------------------===//
 
-#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/Support/raw_ostream.h"
 
 #include "gtest/gtest.h"

Copy link
Contributor

@porglezomp porglezomp left a comment

Choose a reason for hiding this comment

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

LGTM!


using namespace llvm;

TEST(InterleaveTest, Interleave) {
Copy link
Member

Choose a reason for hiding this comment

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

Surround TEST with namespace { ... }

@jroelofs jroelofs merged commit e84a757 into llvm:main Apr 5, 2024
@jroelofs jroelofs deleted the jroelofs/interleave branch April 5, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants