[PATCH] D80586: Refactor Suffix Tree from MachineOutliner to LibSupport

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

Can you make the testcases more explicit?

"This tests that the suffix tree finds (stuff)"

"This tests that the suffix tree does not find (stuff)"



================
Comment at: llvm/unittests/Support/SuffixTreeTest.cpp:19
+// the string
+
+TEST(SuffixTreeTest, TestSingleRepetition) {
----------------
Comment explaining what this testcase does?


================
Comment at: llvm/unittests/Support/SuffixTreeTest.cpp:27
+  }
+  ASSERT_EQ(SubStrings.size(), (unsigned)1);
+  EXPECT_EQ(SubStrings[0].Length, (unsigned)2);
----------------
`static_cast` for casting?


================
Comment at: llvm/unittests/Support/SuffixTreeTest.cpp:32-35
+// Tests for what the Suffix Tree is currently able to do, ideally in the
+// future this result will change to be able to catch more cases where
+// it finds not only the longest repeated substrings, but the substrings within
+// that substring that share the same starting point
----------------
It would be better if these comments described exactly what is being tested instead of "what it is currently able to do".

Also, it would be good to call out explicitly what should not be found in this comment.


================
Comment at: llvm/unittests/Support/SuffixTreeTest.cpp:43
+  EXPECT_EQ(SubStrings.size(), (unsigned)2);
+  for (SuffixTree::RepeatedSubstring RS : SubStrings) {
+    EXPECT_EQ(RS.StartIndices.size(), (unsigned)2);
----------------
Use reference?

```
for (const SuffixTree::RepeatedSubstring &RS : SubStrings)  {
 ...
```


================
Comment at: llvm/unittests/Support/SuffixTreeTest.cpp:44
+  for (SuffixTree::RepeatedSubstring RS : SubStrings) {
+    EXPECT_EQ(RS.StartIndices.size(), (unsigned)2);
+  }
----------------
Can we check the specific substrings which were found?


================
Comment at: llvm/unittests/Support/SuffixTreeTest.cpp:48-51
+// Tests for what the Suffix Tree is currently able to do, ideally in the
+// future this result will change to be able to catch more cases where
+// it finds not only the longest repeated substrings, but the substrings within
+// that substring
----------------
Same here, can this comment describe what the test below is actually testing?


================
Comment at: llvm/unittests/Support/SuffixTreeTest.cpp:64
+}
+
+TEST(SuffixTreeTest, TestExclusion) {
----------------
Comment for below testcase?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80586/new/

https://reviews.llvm.org/D80586