[PATCH] D118132: [demangler] stricter NestedName parsing

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

Thanks for adding me as reviewer. It is good for me to learn about CXXABI. Due to the same reason, maybe I couldn't give a high quality feedback. I read code with your description. I think they are basically consistent with your description. But I couldn't found problem  in the description if any.

BTW, I feel the `if { continue; }` style is more simpler than `if {} else if {} else if{}` style. The cyclomatic complexity is lower. Although there is a little bit redundant, but I feel it more clear and explicit to read and understand.



================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:3173
     if (State) State->ReferenceQualifier = FrefQualLValue;
-  } else
+  } else {
     if (State) State->ReferenceQualifier = FrefQualNone;
----------------
This change looks not necessary.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:3188-3189
 
-    //          ::= <template-param>
+    if (State)
+      State->EndsWithTemplateArgs = false;
+
----------------
I am not sure about the style guide of libcxxabi. But I feel it is necessary to add comment here. I get the logic from your description of the patch, but I can't get it from the code simply.


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:3193-3194
+      //          ::= <template-param>
+      if (SoFar != nullptr)
         return nullptr;
+      SoFar = getDerived().parseTemplateParam();
----------------
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.


================
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)
----------------
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?


================
Comment at: libcxxabi/src/demangle/ItaniumDemangle.h:3215
         return nullptr;
-      SoFar = getDerived().parseAbiTags(SoFar);
+      if (look(1) == 't') {
+        First += 2;
----------------
At least we need a comment some like:
```
::= St [L]* <unqualified-name>   # ::std::
```


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

https://reviews.llvm.org/D118132