[PATCH] D110664: [Symbolize] Demangle Rust symbols

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

tmiasko added inline comments.


================
Comment at: llvm/lib/Demangle/Demangle.cpp:31-32
+
+  if (S[0] == '_' && llvm::nonMicrosoftDemangle(S + 1, Result))
+    return Result;
+
----------------
jhenderson wrote:
> Previously you didn't need this second call. I'm not sure I follow why you need it now?
The llvm::demangle and llvm-cxxfilt differ in a treatment of the leading underscore. For Itanium mangling llvm-cxxfilt accepts only symbols starting with `_Z` or `___Z`, but has an option to strip an extra underscore first. On the other hand llvm::demangle accepts all variants by default, i.e., not only `_Z` and `__Z` but also `__Z` and `____Z`.

To retain the existing behaviour while sharing the code that checks mangling prefixes, the new function expects symbols without an extra underscore. Thus when first attempt at demangling fails, the llvm::demangle tries once again with the leading underscore stripped.

The implementation strategy also subtly changed, since now responsibility for stripping the extra underscore moved from the inside of itaniumDemangle to the outside.


================
Comment at: llvm/lib/Demangle/Demangle.cpp:44
+
+bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) {
+  char *Demangled = nullptr;
----------------
jhenderson wrote:
> Do we have access to `llvm::Optional` here? Not sure, as I know there are some layering issues specific to the demangler.
> 
> If we do, I think returning an optional string would be better than passing in a reference.
The demangle library currently doesn't have any dependencies on other LLVM components. Otherwise the optional string would be my preferred choice here as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110664