[PATCH] D102729: [Demangle][Rust] Parse binders

[David Blaikie via Phabricator via llvm-commits]

dblaikie accepted this revision.
dblaikie added a comment.
This revision is now accepted and ready to land.

Sounds good, thanks!



================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:471-476
+    if (consumeIf('L')) {
+      if (auto Lifetime = parseBase62Number()) {
+        printLifetime(Lifetime);
+        print(' ');
+      }
+    }
----------------
tmiasko wrote:
> dblaikie wrote:
> > tmiasko wrote:
> > > dblaikie wrote:
> > > > Might be nice to have this written once/reused (rather than duplicated here between 'R' and 'Q' for instance) - not sure if it can be generalized over the use in `demangleGenericArg` too - all of them use the <lifetime> Production, but there's no common implementation of/function for that production - maybe there could be?
> > > I reused the implementation between `R` and `Q` productions.
> > any chance of having a "demangleLifetime" that can be used in "demangleGenericArg" as well as here? (nice to have a function for each production, so it's easier to find the implementation of that production in one place)
> Yeah it would be nice to unify those, but there is slight difference in a treatment of erased lifetimes:
> 
> * In generic arguments erased lifetimes are always printed, since order is significant (although rustc does not mangle any lifetimes arguments when all lifetimes are erased).
> * In references they are not printed (rustc does not mangle them in this context, so a difference in demangler behaviour would be unobservable in practice).
> * In trait objects (yet to be implemented) lifetimes are mandatory, but there is no reason to print erased lifetimes.
having parameters to the demangle function to support these different behaviors doesn't seem totally unreasonable, while still having central handling? (or having variants "demangleLifetimeForGenericArg", etc)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102729