[PATCH] D102571: [Demangle][Rust] Parse named types

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Demangle/RustDemangle.h:82
 private:
-  void demanglePath();
+  void demanglePath(bool InType);
   void demangleImplPath();
----------------
Could use a custom enum parameter here to avoid having to add a comment at every call site.


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:424-437
+  char C = consume();
   BasicType Type;
-  if (parseBasicType(consume(), Type))
-    printBasicType(Type);
-  else
-    Error = true; // FIXME parse remaining productions.
+  if (parseBasicType(C, Type))
+    return printBasicType(Type);
+
+  switch (C) {
+  case 'C':
----------------
Perhaps it'd be more appropriate to use `look()` here, then `consume()` in the `if` block below?


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:429-435
+  switch (C) {
+  case 'C':
+  case 'M':
+  case 'X':
+  case 'Y':
+  case 'N':
+  case 'I':
----------------
Rather than special casing a bunch of things here - especially if the `consume`->`look` change makes sense - could this code here use `demanglePath` unconditionally and let errors fall out of that naturally? (it'd match the grammar more closely, I think - avoid needing the implementation of `<path>` mixed up in the implementation of `<type>`)

I guess maybe that's not feasible - if some invalid basic type could then be interpreted as a path, even though it should be an invalid basic type instead?


================
Comment at: llvm/test/Demangle/rust.test:141
+CHECK: <name>
+       _RMC5crateC4name
+
----------------
These tests for `<type>` are using `inherited impl` whereas I think previous tests for `<type>` were using `generic args`. It'd be nice if they were consistent, I think? (so as not to raise questions/confusion about whether the context in which the type appears is significant to the testing of the type rule) (I don't mind which way they are consistent - if you'd rather change the previous tests over to use `inherited impl` so they're consistent with the new tests, or chan these new ones to use `generic args` - or both to some third thing (changing existing tests should be done in a preliminary/separate commit, though))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102571