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.

cfe-commits mailing list

Reply via email to