[PATCH] D103151: [Demangle][Rust] Parse dyn-bounds

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

tmiasko added inline comments.


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:485-492
+    if (consumeIf('L')) {
+      if (auto Lifetime = parseBase62Number()) {
+        print(" + ");
+        printLifetime(Lifetime);
+      }
+    } else {
+      Error = true;
----------------
dblaikie wrote:
> keep thinking it'd be nice if this were wrapped up in some (albeit parameterized for different situations you mentioned) "demangleLifetime" to match the syntax description more closely
I am still somewhat reluctant to change this, since I would have to introduce three different variants  and each would be used only once. For example, in variant here, when lifetime is printed it should be preceded by ` + `.


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:556
 // <binder> = ["G" <base-62-number>]
 void Demangler::demangleBinder() {
   uint64_t Binder = parseOptionalBase62Number('G');
----------------
dblaikie wrote:
> Not sure how consistently this is done throughout the code - but maybe including "optional" when a feature is optional (so it's clear it's implementing `[<binder>]` not `<binder>`) would be good?
I renamed the function in parent revision to `demangleOptionalBinder`.


================
Comment at: llvm/test/Demangle/rust.test:243-244
+
+CHECK: trait::<for<'a> fn(dyn for<'b> + 'a)>
+       _RIC5traitFG_DG_EL0_EuE
+
----------------
dblaikie wrote:
> Could this test be simplified down to:
> ```
> CHECK: trait::<dyn for<'b> + 'a>
>        _RIC5traitDG_EL_E
> ```
In `"D" <dyn-bounds> <lifetime>` production, `<lifetime>` is outside the binder from `<dyn-bounds>`. Additionally since erased lifetimes are not printed here, test has to use another binder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103151