[llvm-commits] Twines

[Chris Lattner]

On Jul 23, 2009, at 12:26 AM, Daniel Dunbar wrote:

> Here are the patches to add "Twines" to LLVM, as described on
> llvm-dev. The first patch is the Twine data structure, the second
> patch changes llvm::Value to use it -- it mostly amounts to a search
> and replace of 'const std::string &' to 'const Twine &' over the
> relevant APIs.
>
> These are basically the minimal delta to implement and test them, to
> verify they are effective. Two features are missing:
> 1. llvm::Value does not yet use the 'null' Twine.
> 2. There is no direct support for concatenating integers-as-strings.

Ok.

> There are also a number of hacks to make introducing Twines easier,
> which should be removed:
> 1. Twine shouldn't actually have a conversion operator to std::string.
> 2. The patch hacks in support for writing StringRef's to
> std::ostream, we should move clients to raw_ostream instead.
> 3. We probably don't want Twine.h in Value.h, clients should
> explicitly import it if they want to get implicit string
> concatenation.


You're planning to do this as a follow-on, right?  It would also be  
nice to remove a bunch of the Value::*Name* methods.

Please also add a short mention of this in the ProgrammersManual.html  
file (datastructure section).

+++ include/llvm/ADT/Twine.h	(revision 0)

+  /// A Twine is a kind of rope, it represents a concatenated string  
by a
+  /// binary-tree, where the string is the preorder of the nodes.  
Since the

"by a" -> "with a" or "as a"

+  // FIXME: We could normalize "" (in its C string, StringRef, and  
std::string)
+  // spellings to Empty. I decided this was not worth the effort,  
complexity, or
+  // increased code size; clients rarely concatenate empty strings?

I agree, don't worry about empty strings.

+    /// LHSKind - The NodeKind of the left hand side, \see  
getLHSKind().
+    unsigned LHSKind : 8;
+    /// RHSKind - The NodeKind of the left hand side, \see  
getLHSKind().
+    unsigned RHSKind : 8;

Please declare as "NodeKind LHSKind : 8;"

+++ lib/Support/Twine.cpp	(revision 0)
@@ -0,0 +1,83 @@
+//===-- Twine.h - Fast Temporary String Concatenation  
---------------------===//

Please s/.h/.cpp.

+std::string Twine::str() const {
+  std::string Res;
+  raw_string_ostream OS(Res);
+  print(OS);
+  return Res;
+}

Do you anticipate the performance of this method to matter?  If so,  
you could do a pre-traversal to get a bound for the length of the  
resultant string, then do a reserve, then get the contents.

+  case Twine::CStringKind:
+    OS << static_cast<const char*>(Ptr); break;
+  case Twine::StdStringKind:
+    OS << *static_cast<const std::string*>(Ptr); break;
+  case Twine::StringRefKind:
+    OS << *static_cast<const StringRef*>(Ptr); break;
+  case Twine::TwineKind:
+    static_cast<const Twine*>(Ptr)->print(OS); break;

Please put the breaks on their own line unless the whole statement is  
on one line with the 'case'.


+++ include/llvm/ADT/StringRef.h	(working copy)
@@ -171,6 +171,16 @@

    /// @}

+  struct StringRefWrapper {
+    const StringRef &Str;
+    StringRefWrapper(const StringRef &_Str) : Str(_Str) {}
+  };
+
+  template<typename T>
+  T &operator<<(T &OS, const StringRefWrapper &RHS) {
+    OS.write(RHS.Str.data(), RHS.Str.size());
+    return OS;
+  }
  }

Why do you need StringRefWrapper?  I assume this is due to ambiguity,  
please add a comment.


+++ tools/bugpoint/ExtractFunction.cpp	(working copy)
@@ -353,7 +353,8 @@
      // If the BB doesn't have a name, give it one so we have  
something to key
      // off of.
      if (!BB->hasName()) BB->setName("tmpbb");
-    BlocksToNotExtractFile << BB->getParent()->getName() << " "
+    // FIXME: Remove explicit .str() call once ambiguous overload  
goes away.
+    BlocksToNotExtractFile << BB->getParent()->getName().str() << " "
                             << BB->getName() << "\n";
    }
    BlocksToNotExtractFile.close();
Index: unittests/ADT/StringRefTest.cpp
===================================================================
--- unittests/ADT/StringRefTest.cpp	(revision 76864)
+++ unittests/ADT/StringRefTest.cpp	(working copy)
@@ -70,7 +70,8 @@

    std::string Storage;
    raw_string_ostream OS(Storage);
-  OS << StringRef("hello");
+  // FIXME: Remove .str() when overload is fixed.
+  OS << StringRef("hello").str();
    EXPECT_EQ("hello", OS.str());
  }

When will the fixme's be resolved?


Overall, this is very very cool.  Thank you for working on this!

-Chris