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

[Tomasz Miąsko via Phabricator via llvm-commits]

tmiasko added inline comments.


================
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':
----------------
dblaikie wrote:
> Perhaps it'd be more appropriate to use `look()` here, then `consume()` in the `if` block below?
In context of this changes using `look()` instead of `consume()` seems like an improvement, and I changed it accordingly. 

Admittedly I will return to the previous approach when introducing additional type productions since each would have to consume the initial character otherwise.


================
Comment at: llvm/test/Demangle/rust.test:141
+CHECK: <name>
+       _RMC5crateC4name
+
----------------
dblaikie wrote:
> 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))
Replaced with generic arguments for consistency. As additional benefit it emphasizes the distinction between demangling of generic arguments outside types where `::` is included, and in types where it is omitted.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102571