[PATCH] D99981: [demangler] Support the new Rust mangling scheme (v0)

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/Demangle/RustDemangle.cpp:25-29
+  if (MangledName == nullptr || (Buf != nullptr && N == nullptr)) {
+    if (Status != nullptr)
+      *Status = demangle_invalid_args;
+    return nullptr;
+  }
----------------
tmiasko wrote:
> dblaikie wrote:
> > jhenderson wrote:
> > > FWIW, I don't think there's a way to test this block without unit-testing, but I'm fine if the bulk wants to be in the lit tests. Same goes with a number of the other potential failure paths.
> > Yeah, API usage issues like this should be exercised by a unit test.
> > 
> > Though I wonder if it might be simpler to not support this and instead assert that the parameters are non-null as needed, rather than that the caller would get some error handling result.
> The main motivation behind the current API choice was to facilitate further integration. Places where I would like to eventually support Rust demangling use itaniumDemangle for the most part, so I settled on the same API. The microsoftDemangle has similar API as well. 
> 
> This API does unfortunately require a bit of extra code and unit tests to cover it (tests are in RustDemangleTest.cpp).
> 
Ah, fair enough.

The existing API also has some quirky memory management which is unfortunate (passing in malloc'd memory and reallocing, etc, internally) - but such is life.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99981