[PATCH] D36427: [libcxxabi][demangler] Improve representation of substitutions/templates

Mon Aug 7 17:11:03 PDT 2017

dexonsmith requested changes to this revision.
dexonsmith added a comment.
This revision now requires changes to proceed.

This looks like a great improvement.  I've littered the patch with nit-picks, but my main concern is that there aren't any unit tests for the new data structures.  I wonder if there's a hacky way to set that up...

================
Comment at: src/cxa_demangle.cpp:1460-1461

+template <class T, size_t N>
+class PODSmallVector {
+  static_assert(std::is_pod<T>::value, "");
----------------
This seems like a facility that should/could be unit tested (in C++).

I realize there's no precedent for that for the demangler.  Perhaps this file could be `#include`d by a test file:

```
// unittest/PODSmallVectorTest.h
#include "../src/cxa_demangle.cpp"

namespace {

TEST(PODSmallVectorTest, ...) {
}

} // namespace
```

Thoughts?

================
Comment at: src/cxa_demangle.cpp:1462
+class PODSmallVector {
+  static_assert(std::is_pod<T>::value, "");
+
----------------
Please add an assertion string, such as `"T is required to be a plain-old data type"`.

================
Comment at: src/cxa_demangle.cpp:1499-1501
+        First = Inline;
+        Last = Inline;
+        Cap = Inline + N;
----------------
This is identical to code in the move constructor.  Separate it out into a helper?

================
Comment at: src/cxa_demangle.cpp:1511-1517
+    First = Other.First;
+    Last = Other.Last;
+    Cap = Other.Cap;
+
+    Other.First = Other.Inline;
+    Other.Last = Other.Inline;
+    Other.Cap = Other.Inline + N;
----------------
Helper?

================
Comment at: src/cxa_demangle.cpp:1527-1535
+    size_t S = size();
+    if (!isInline()) {
+      First = static_cast<T*>(std::realloc(First, sizeof(T) * S * 2));
+    } else {
+      First = static_cast<T*>(std::malloc(sizeof(T) * S * 2));
+      std::copy(std::begin(Inline), std::end(Inline), First);
+    }
----------------
Should we assert when `nullptr` is returned from `std::malloc` or `std::realloc`?

================
Comment at: src/cxa_demangle.cpp:1540-1542
+  void pop_back() { --Last; }
+
+  void dropBack(size_t Index) { Last = First + Index; }
----------------
Assertions on popping/dropping past empty?

================
Comment at: src/cxa_demangle.cpp:1549-1550
+  size_t size() const { return static_cast<size_t>(Last - First); }
+  T& back() { return *(Last - 1); }
+  T& operator[](size_t Index) { return *(begin() + Index); }
+  void clear() { Last = First; }
----------------
Assertions on invalid access?

================
Comment at: src/cxa_demangle.cpp:1562
+template<size_t Size>
+class SubTable {
+  // Subs hold the actual entries in the table, and PackIndices tells us which
----------------
As I read this patch, I keep reading "sub" as a prefix.  Can we just spell this out as "SubstitutionTable"?

Also, this probably deserves unit tests as well.

================
Comment at: src/cxa_demangle.cpp:1577-1580
+  void pushSub(Node* Entry) {
+    PackIndices.push_back(static_cast<unsigned>(Subs.size()));
+    Subs.push_back(Entry);
+  }
----------------
Can/should this be implemented using `pushPack()` and `pushSubIntoPack()`?

================
Comment at: src/cxa_demangle.cpp:1585
+  void pushPack() { PackIndices.push_back(static_cast<unsigned>(Subs.size())); }
+  void pushSubIntoPack(Node* Entry) { Subs.push_back(Entry); }
+
----------------
Do you need to assert that `pushPack` has been called at least once (i.e., `!PackIndices.empty()`)?

================
Comment at: src/cxa_demangle.cpp:1595
+  // For use in a range-for loop.
+  struct NodePair {
+    Node **First;
----------------
Perhaps `NodeRange` would be more clear.

================
Comment at: src/cxa_demangle.cpp:1596-1599
+    Node **First;
+    Node **Last;
+    Node **begin() { return First; }
+    Node **end() { return Last; }
----------------
Please be consistent with pointer style in this patch.
- I suspect the rest of the file attaches to the type, i.e., `Node** First`, which is why I didn't comment earlier on, e.g., `Node* Entry`.
- LLVM style is to attach to the name, like here.

I have a slight preference for switching toward LLVM style, but if the file consistently uses pointers attached to types, that's fine too.  (Just be consistent within the patch.)

================
Comment at: src/cxa_demangle.cpp:1602
+
+  NodePair nthSub(size_t N) {
+    assert(N < PackIndices.size());
----------------
This could perhaps use a comment.  At least, inside the function to explain the math.

================
Comment at: src/cxa_demangle.cpp:1603
+  NodePair nthSub(size_t N) {
+    assert(N < PackIndices.size());
+    Node **Begin = Subs.begin() + PackIndices[N];
----------------
Once you add this assertion to `operator[]` you can delete it here.

================
Comment at: src/cxa_demangle.cpp:1604
+    assert(N < PackIndices.size());
+    Node **Begin = Subs.begin() + PackIndices[N];
+    Node **End = (N + 1 == PackIndices.size())
----------------
Should there be some assertion on `Subs.size()` here?  Perhaps, if so, this should be `&Subs[PackIndices[N]]`, so that `Subs.operator[]()` handles it for you.  Same below.

================
Comment at: src/cxa_demangle.cpp:1605-1607
+    Node **End = (N + 1 == PackIndices.size())
+                     ? Subs.end()
+                     : Subs.begin() + PackIndices[N + 1];
----------------
Does this deserve its own helper function (e.g., `packEnd()`)?  (I haven't read ahead to see if this logic is duplicated anywhere; probably not a separate function if it's not duplicated.)

================
Comment at: src/cxa_demangle.cpp:1621-1631
+    // Name stack, this is used by the parser to hold temporary names that were
+    // parsed. The parser colapses multiple names into new nodes to construct
+    // the AST. Once the parser is finished, names.size() == 1.
+    PODSmallVector<Node*, 32> names;
+
+    // Substitution table. Itanium supports name substitutions as a means of
+    // compression. The string "S42_" refers to the 42nd entry in this table.
----------------
How much have you thought about these sizes (32, 32, and 4)?  Any evidence to back up these specific choices?

https://reviews.llvm.org/D36427