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