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

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

tmiasko added inline comments.


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:471-476
+    if (consumeIf('L')) {
+      if (auto Lifetime = parseBase62Number()) {
+        printLifetime(Lifetime);
+        print(' ');
+      }
+    }
----------------
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.


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:707
 // Parses optional base 62 number. The presence of a number is determined using
 // Tag. Returns 0 when tag is absent and parsed value + 1 otherwise.
 uint64_t Demangler::parseOptionalBase62Number(char Tag) {
----------------
dblaikie wrote:
> tmiasko wrote:
> > dblaikie wrote:
> > > This seems like a really subtle and confusing API, especially given the existing "+ 1" behavior of the Base 62 encoding. I've been staring at the code in demangleFnSig+demangleBinder to figure out the first binder test case for a while before I realized that.
> > > 
> > > Any chance this could use uintmax for the invalid value, rather than offsetting the result in this way?
> > There are two layers of `+1` which makes it quite confusing, but this is intended encoding of optional base62 numbers.
> Fair enough - maybe some more quotations from the spec to explain this would be useful?
I extended the comment and added a few concrete examples.


================
Comment at: llvm/test/Demangle/rust.test:237-238
+
+CHECK: binders::<for<'a> fn(&'a _)>
+       _RIC7bindersFG_RL0_pEuE
+
----------------
dblaikie wrote:
> tmiasko wrote:
> > dblaikie wrote:
> > > I'm having a hard time understanding this test in the context of the spec. 
> > > 
> > > The spec says that `_` encodes zero. So the `G_` should be zero, right? Is there somewhere in the spec that says that `<binder>` mangled value is then 1 larger than that? (so a binder value 0 (`_`) is actually for 1 binder)?
> > This looks like an omission in the specification The binder production should be marked as optional, and then the presence of `G` participates in the encoding of the value in analogous way to `s` in disambiguators:
> > 
> > > If the disambiguator is not present, this index is 0. If it is of the form s_ then the index is 1. If it is of the form s<base-62-digit>_ then the index is <base-62-digit> + 2.
> > 
> > Does this clarify the situation?
> > 
> > I will look into updating the specification.
> Ah, OK - yeah, spec update and some more words in the implementation (see other comment) would probably be handy. Thanks!
The proposed update to the specification https://github.com/rust-lang/rfcs/pull/3130.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102729