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