[PATCH] D41885: [libcxxabi][demangler] Improve handling of variadic templates

[Erik Pilkington via Phabricator via cfe-commits]

erik.pilkington added inline comments.


================
Comment at: src/cxa_demangle.cpp:260-261
+
+#if 0
+  void dump() const {
+    char *Buffer = static_cast<char*>(std::malloc(1024));
----------------
dexonsmith wrote:
> Why is this behind `#if 0`?  Should you just use something like `LLVM_DUMP_METHOD`?
Do you mean conditionally defined based on NDEBUG and marked __attribute__((noinline,used)) for debuggers? I was just using it as a quick & simple little helper for debugging.


================
Comment at: src/cxa_demangle.cpp:1412-1414
+    setCachedRHSComponent(false);
+    setCachedArray(false);
+    setCachedFunction(false);
----------------
dexonsmith wrote:
> This seems like a super-common sequence in constructors of subclasses of `Node`.  Can you pull out a common subclass of `Node` (say, `CachingNode`)?
I had a hard time finding a nice way of abstracting the cache from the individual ctors, they all need to set it up based on their parameters. The new patch just defaults to setting the three cached values to false, with each ctor overriding when necessary. This gets rid of a lot of the duplication.


================
Comment at: test/test_demangle.pass.cpp:29607
     {"_ZTHN3fooE", "thread-local initialization routine for foo"},
-    {"_Z4algoIJiiiEEvZ1gEUlT_E_", "void algo<int, int, int>(g::'lambda'(int, int, int))"},
+    {"_Z4algoIJiiiEEvZ1gEUlDpT_E_", "void algo<int, int, int>(g::'lambda'(int, int, int))"},
     // attribute abi_tag
----------------
dexonsmith wrote:
> erik.pilkington wrote:
> > I just generated this symbol by hand because I couldn't get clang to do it without crashing. Turns out I forgot the Dp!
> Why was it crashing?  (Did you file a bug?)
Yep, I filed https://bugs.llvm.org/show_bug.cgi?id=33092 to track this at the time. I didn't really look too far into the crash, but it seems pretty low-priority.


https://reviews.llvm.org/D41885