[PATCH] D118132: [demangler] stricter NestedName parsing

[Nathan Sidwell via Phabricator via llvm-commits]

urnathan marked 4 inline comments as done.
urnathan added inline comments.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:3193-3194
+      //          ::= <template-param>
+      if (SoFar != nullptr)
         return nullptr;
+      SoFar = getDerived().parseTemplateParam();
----------------
ChuanqiXu wrote:
> Similarly, I can get this from your description that "In a nested name, a <template-param>, <decltype> or <substitution>
> can only appear as the first element." I think it would be helpful to add a comment. Personally, I think it would be more easy to read if we move this outside the loop if we want enforce something that could only be at the start.
It's a semantic restriction.  I've added a comment, but I'm not totally wedded to this particular change.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:3201-3202
         return nullptr;
-      SoFar = make<NameWithTemplateArgs>(SoFar, TA);
-      if (!SoFar)
-        return nullptr;
-      if (State) State->EndsWithTemplateArgs = true;
-      Subs.push_back(SoFar);
-      continue;
-    }
-
-    //          ::= <decltype>
-    if (look() == 'D' && (look(1) == 't' || look(1) == 'T')) {
-      if (!PushComponent(getDerived().parseDecltype()))
+      if (SoFar->getKind() == Node::KNameWithTemplateArgs)
         return nullptr;
+      if (State)
----------------
ChuanqiXu wrote:
> This line might require further knowledge. Does it implies that if the kind of `SoFar` is not `Node::KNameWithTemplateArgs`, the result of `make<NameWithTemplateArgs>(SoFar, TA);` wouldn't be nullptr?
There is no such inference.  Not sure what you're asking here.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:3215
         return nullptr;
-      SoFar = getDerived().parseAbiTags(SoFar);
+      if (look(1) == 't') {
+        First += 2;
----------------
ChuanqiXu wrote:
> At least we need a comment some like:
> ```
> ::= St [L]* <unqualified-name>   # ::std::
> ```
That;s not the grammar being parsed though.  This is simply a substitution that parseSubstitution does not handle.  I've added comments.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D118132/new/

https://reviews.llvm.org/D118132