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

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: src/cxa_demangle.cpp:1575-1577
-    sub_type names;
-    template_param_type subs;
-    Vector<template_param_type> template_param;
----------------
dexonsmith wrote:
> - Why not rename `names` as well?
> - Please rename these in a separate prep commit to minimize noise on the final patch.
Sure, r310415.


================
Comment at: src/cxa_demangle.cpp:1604
+    assert(N < PackIndices.size());
+    Node **Begin = Subs.begin() + PackIndices[N];
+    Node **End = (N + 1 == PackIndices.size())
----------------
dexonsmith wrote:
> 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.
We can't use operator[] here because if all we have is an empty parameter pack then `&Subs[PackIndices[N]]` needs to point the the end() iterator, which is out of bounds. The new patch adds an assert though.


================
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.
----------------
dexonsmith wrote:
> How much have you thought about these sizes (32, 32, and 4)?  Any evidence to back up these specific choices?
Yep, 32 is enough for the Subs table/names to never overflow when demangling the symbols in LLVM, and 4 seldom overflows TemplateParams. TemplateParams is temporary held on the stack in parse_template_args(), so I wanted it to be smaller.


https://reviews.llvm.org/D36427