Skip to content
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

[ADT] Fix llvm::join on containers of char*s #67113

Merged
merged 3 commits into from
Sep 28, 2023
Merged

Conversation

sam-mccall
Copy link
Collaborator

Currently it tries to call S.size() when preallocating the target string,
which doesn't compile.
vector<const char*> is used a bunch in e.g. clang driver.

Currently it tries to call S.size() when preallocating the target string,
which doesn't compile.
vector<const char*> is used a bunch in e.g. clang driver.
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 22, 2023

@llvm/pr-subscribers-llvm-adt

Changes

Currently it tries to call S.size() when preallocating the target string,
which doesn't compile.
vector<const char*> is used a bunch in e.g. clang driver.


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

2 Files Affected:

  • (modified) llvm/include/llvm/ADT/StringExtras.h (+8-2)
  • (modified) llvm/unittests/ADT/StringExtrasTest.cpp (+15-2)
diff --git a/llvm/include/llvm/ADT/StringExtras.h b/llvm/include/llvm/ADT/StringExtras.h
index 30397b23ab03b37..b2c400f512695e8 100644
--- a/llvm/include/llvm/ADT/StringExtras.h
+++ b/llvm/include/llvm/ADT/StringExtras.h
@@ -26,6 +26,7 @@
 #include <cstring>
 #include <iterator>
 #include <string>
+#include <type_traits>
 #include <utility>
 
 namespace llvm {
@@ -417,8 +418,13 @@ inline std::string join_impl(IteratorT Begin, IteratorT End,
     return S;
 
   size_t Len = (std::distance(Begin, End) - 1) * Separator.size();
-  for (IteratorT I = Begin; I != End; ++I)
-    Len += (*I).size();
+  for (IteratorT I = Begin; I != End; ++I) {
+    if constexpr (std::is_same_v<std::remove_reference_t<decltype(*I)>,
+                                 const char *>)
+      Len += strlen(*I);
+    else
+      Len += (*I).size();
+  }
   S.reserve(Len);
   size_t PrevCapacity = S.capacity();
   (void)PrevCapacity;
diff --git a/llvm/unittests/ADT/StringExtrasTest.cpp b/llvm/unittests/ADT/StringExtrasTest.cpp
index 971560b8eb8c4ee..0fead285e3d9d8a 100644
--- a/llvm/unittests/ADT/StringExtrasTest.cpp
+++ b/llvm/unittests/ADT/StringExtrasTest.cpp
@@ -59,8 +59,8 @@ TEST(StringExtrasTest, isUpper) {
   EXPECT_FALSE(isUpper('\?'));
 }
 
-TEST(StringExtrasTest, Join) {
-  std::vector<std::string> Items;
+template <class ContainerT> void testJoin() {
+  ContainerT Items;
   EXPECT_EQ("", join(Items.begin(), Items.end(), " <sep> "));
 
   Items = {"foo"};
@@ -74,6 +74,19 @@ TEST(StringExtrasTest, Join) {
             join(Items.begin(), Items.end(), " <sep> "));
 }
 
+TEST(StringExtrasTest, Join) {
+  {
+    SCOPED_TRACE("std::vector<std::string>");
+    testJoin<std::vector<std::string>>();
+  }
+  {
+    SCOPED_TRACE("std::vector<const char*>");
+    testJoin<std::vector<const char *>>();
+  }
+}
+
+TEST(StringExtrasTest, JoinCStrings) { std::vector<const char *> Items; }
+
 TEST(StringExtrasTest, JoinItems) {
   const char *Foo = "foo";
   std::string Bar = "bar";

@sam-mccall sam-mccall merged commit 898b961 into llvm:main Sep 28, 2023
3 checks passed
legrosbuffle pushed a commit to legrosbuffle/llvm-project that referenced this pull request Sep 29, 2023
Currently it tries to call S.size() when preallocating the target
string,
which doesn't compile.
vector<const char*> is used a bunch in e.g. clang driver.
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.

None yet

4 participants