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

[Duncan P. N. Exon Smith via Phabricator via cfe-commits]

dexonsmith accepted this revision.
dexonsmith added a comment.
This revision is now accepted and ready to land.

I have a few more nitpicks; LGTM after you fix those.



================
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.
----------------
erik.pilkington wrote:
> 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.
Sounds great.  Please add comments to that effect.


================
Comment at: src/cxa_demangle.cpp:1626-1630
+    Node **Begin = Substitutions.begin() + PackIndices[N];
+    Node **End = (N + 1 == PackIndices.size())
+                     ? Substitutions.end()
+                     : Substitutions.begin() + PackIndices[N + 1];
+    assert(End <= Substitutions.end() && Begin <= End);
----------------
This assertion won't be meaningful in the case that matters: when `PackIndices[N + 1]` is larger than `Substitutions.size()`.  In that case, `End` is in the middle of nowhere, and the result of `End <= Substitutions.end()` is undefined.  A smart optimizer would fold that to `true`.

You should assert `PackIndices[N + 1] <= Substitutions.size()`.


================
Comment at: src/cxa_demangle.cpp:5195-5199
+                auto TmpParams = std::move(db.TemplateParams);
+                size_t k0 = db.Names.size();
+                const char* t1 = parse_template_arg(t, last, db);
+                size_t k1 = db.Names.size();
+                db.TemplateParams = std::move(TmpParams);
----------------
This looks really hairy.  This is the only (non-recursive) caller of `parse_template_arg`... it doesn't seem reasonable for `parse_template_arg` to modify template params, if we're just going to reset them after without looking.

Please add a FIXME to sort this out (at least to document the logic, if not to refactor it somehow).


================
Comment at: src/cxa_demangle.cpp:5207-5209
+            }
+            else
+            {
----------------
Early `continue` instead of `else`, if you leave the logic like this.

I'm a little skeptical that this structure is better than before, though, since there's some non-trivial duplicated code.  I'll let you decide.


https://reviews.llvm.org/D36427