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 

cfe-commits mailing list

Reply via email to