[PATCH] D26870: Outliner: Add uniquely terminated strings for a suffix tree

[Erik Pilkington via llvm-commits]

erik.pilkington added inline comments.


================
Comment at: include/llvm/ADT/TerminatedString.h:161
+
+template <typename InputContainer, typename CharacterType>
+struct TerminatedString {
----------------
MatzeB wrote:
> Generally: Does this class have enough interesting behaviour of just a SmallVector<CharacterType>/std::vector<Character>? Just providing a few convinience functions for the few bits that differ from a normal container may be the easier and more flexible solution.
Why have this type parameterized on InputContainer? Looks like you only use it for operator+= and the constructor, It might be nice to just template those functions for more generality!


================
Comment at: include/llvm/ADT/TerminatedString.h:172
+
+  /// Note that size and length have different meanings. SizeStrings() includes
+  /// the
----------------
This seems kinda subtle... Maybe we should follow what the iterators do here, ie, have size() and charsSize()? (also, please fix the formatting of the next line!)


================
Comment at: include/llvm/ADT/TerminatedString.h:262
+
+    for (size_t i = 0, e = Rhs.size(); i < e; i++) {
+      CharLike Ch;
----------------
Might be cleaner to use StrContainer.insert(), then set the last element to not be a terminator?


================
Comment at: include/llvm/ADT/TerminatedString.h:280
+
+    for (size_t i = 0, e = Rhs.length(); i < e; i++) {
+      CharLike Ch;
----------------
Same here!


================
Comment at: include/llvm/ADT/TerminatedString.h:295
+  /// TerminatedString
+  void erase(const size_t &Idx) {
+    assert(Idx != length() && "Can't erase terminator! (Idx == length())");
----------------
Any reason not to just pass Idx by value here?


================
Comment at: include/llvm/ADT/TerminatedString.h:343
+  /// Create a copy of another TerminatedString, including the terminator.
+  TerminatedString(const TerminatedString &other) {
+    for (size_t i = 0; i < other.size(); i++)
----------------
This should just be: `TerminatedString(const TerminatedString &Other): StrContainer(Other.StrContainer) {}`


================
Comment at: include/llvm/ADT/TerminatedString.h:384
+  size_t SizeChars;   // Sum of size() for each string in list
+  size_t SizeStrings; // Number of strings in the list
+
----------------
Why do we have this member? Can't we just use StringContainer.size()?


================
Comment at: include/llvm/ADT/TerminatedString.h:387
+  typedef TerminatedString<InputContainer, CharLike> String;
+  typedef std::vector<String *> StringContainer;
+  typedef Character<CharLike> CharacterType;
----------------
Are we responsible for deleteing the Strings? If so this should probably be a `unique_ptr<String>`.


================
Comment at: include/llvm/ADT/TerminatedString.h:431
+  /// back(): Return the element at the end of the TerminatedStringList.
+  String back() { return *(Str.back()); }
+
----------------
You probably want to return by reference here?


================
Comment at: include/llvm/ADT/TerminatedString.h:445
+  void append(String &str_) {
+    Str.push_back(new String(str_));
+    SizeChars += str_.size();
----------------
no delete for this new!


Repository:
  rL LLVM

https://reviews.llvm.org/D26870