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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits