[PATCH] D51205: Move SuffixTree type to a common location

[Michael Spencer via Phabricator via llvm-commits]

Bigcheese requested changes to this revision.
Bigcheese added inline comments.
This revision now requires changes to proceed.


================
Comment at: include/llvm/ADT/SuffixTree.h:20
+
+namespace {
+
----------------
You shouldn't use anonymous namespaces in headers as it means each TU that includes it gets a different copy of the code.  This should be `namespace llvm`.


================
Comment at: include/llvm/ADT/SuffixTree.h:23
+/// Represents an undefined index in the suffix tree.
+const unsigned EmptyIdx = -1;
+
----------------
This was fine in the cpp file, but shouldn't be added to the global or llvm namespace in a header.  It should probably be moved into `SuffixTree` or `SuffixTreeNode`.


================
Comment at: include/llvm/ADT/SuffixTree.h:294
+  /// this step.
+  unsigned extend(unsigned EndIdx, unsigned SuffixesToAdd) {
+    SuffixTreeNode *NeedsLink = nullptr;
----------------
This function body should be moved out to its own .cpp file.  The general rule is any definition of a non-template function of more than a few statements should be in a .cpp file instead of the header.


Repository:
  rL LLVM

https://reviews.llvm.org/D51205