[PATCH] D40279: [libcxxabi][demangler] Add demangling for __attribute__((abi_tag))

[Eric Fiselier via Phabricator via cfe-commits]

EricWF added inline comments.


================
Comment at: src/cxa_demangle.cpp:10
 
+// FIXME: (possibly) incomplete list of features that clang mangles that this
+// file does not yet support:
----------------
Awesome comment!

If your awesomeness knows no bound, I would love to add currently-failing tests that demonstrate the FIXME.
Obviously that has no bearing on how this patch proceeds. 


================
Comment at: src/cxa_demangle.cpp:16
+
 #define _LIBCPP_NO_EXCEPTIONS
 
----------------
Urg...

Entirely unrelated to this patch, but manually defining `_LIBCPP_NO_EXCEPTIONS` here smells awful.


================
Comment at: src/cxa_demangle.cpp:1848
+// <abi-tag>     ::= B <positive length number> <identifier>
+
+const char*
----------------
I realize the unneeded blank line is part of the existing style... But I fudging hate it.

If you're not opposed I would love to start removing the unneeded line wherever it's
part of this changeset.


================
Comment at: src/cxa_demangle.cpp:4353
 //                  ::= C3    # complete object allocating constructor
 //   extension      ::= C5    # ?
 //                  ::= D0    # deleting destructor
----------------
Does this comment need updating now that `<ctor-dtor-name>` now parses with an optional ABI tag at the end?


================
Comment at: src/cxa_demangle.cpp:4408
 // <unnamed-type-name> ::= Ut [ <nonnegative number> ] _
 //                     ::= <closure-type-name>
 // 
----------------
Does this comment need to be updated to reflect that `<unnamed-type-name>` now parses with an optional ABI tag after the `_` character?


================
Comment at: test/test_demangle.pass.cpp:29608
     {"_Z4algoIJiiiEEvZ1gEUlT_E_", "void algo<int, int, int>(g::'lambda'(int, int, int))"},
+
+    {"_Z1fB3foov", "f[abi:foo]()"},
----------------
Perhaps it would be better to replace this blank line with a comment stating the tests below are for the ABI tag extension?


https://reviews.llvm.org/D40279