[PATCH] D101444: [demangler] Initial support for the new Rust mangling scheme

[David Blaikie via Phabricator via llvm-commits]

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

Looks good - thanks!



================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:140-141
+  case 'N': {
+    const char NS = consume();
+    if (!isLower(NS) && !isUpper(NS)) {
+      Error = true;
----------------
tmiasko wrote:
> dblaikie wrote:
> > tmiasko wrote:
> > > dblaikie wrote:
> > > > Is this correct/am I understanding this correctly, that some parsed components are not rendered in the demangling? Is that intentional/valid - that multiple mangled names could demangle to the same string/be ambiguous? Or is this a case of some feature not being implemented yet? 
> > > Yes, some components are not shown.
> > > 
> > > In the case here we have two kinds of namespaces: implementation internal namespaces using lowercase letters, and special well-known namespaces using uppercase letters. The former are not intended to be shown. The latter currently includes closures `C` and shims `S` and are shown by the existing demanglers.
> > > 
> > > This is yet to be implemented. I added a FIXME.
> > I see this notion is referenced in the mangling spec document ("Like other disambiguation information, this path would usually not actually be shown by demanglers.") but this is still quite surprising to me - is there precedent for this, do any itanium or Windows manglings produce ambiguous demanglings?
> > 
> > If there isn't precedent for this, I think it'd be worth having a broader discussion about it (maybe a llvm-dev thread).
> One case in itanium that results in the same `f()::a` demangling for both
> statics, would be the following:
> 
> ```
> void f() {
>   { static int a; }
>   { static int a; }
> }
> ```
> 
> The symbols corresponding to constructors & destructors in itanium are
> ambiguous after demangling as well.
Oh, right - thanks for the examples!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101444