[PATCH] D101821: Demangle Rust paths, basic types and named types

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Any chance this could be subdivided further to implement only one grammar rule or the like? It'd make it much easier to inspect the test quality with smaller increments like that.



================
Comment at: llvm/include/llvm/Demangle/RustDemangle.h:84-85
+  template <typename Callable>
+  void withPrint(const bool NewPrint, Callable Demangler) {
+    const auto SavedPrint = Print;
+    Print = NewPrint;
----------------
We tend not to use 'const' on locals, and maybe especially not on parameters (since it's even more readily confused with const-ref on parameters).


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:272-313
+  case 'a':
+    return "i8";
+  case 'b':
+    return "bool";
+  case 'c':
+    return "char";
+  case 'd':
----------------
This whole thing is alphabetic except for 'p' - maybe put it in order?

Also - maybe use a const char* array indexed on `C - 'a'`? Not necessary by any means, though.


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:332-361
+  if (Error || RecursionLevel >= MaxRecursionLevel) {
+    Error = true;
+    return;
+  }
+  RecursionLevel += 1;
+
+  char C = consume();
----------------
Perhaps the recursion system could be a scoped device, making it easier to add/less error-prone in case of early returns, etc?
```
Recurse R(Error, RecursionLimit); // or just take *this and befriend for access to Error and RecursionLimit
if (!R)
  return;
// Recurse's dtor decrements the RecursionLimit)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101821