[Lldb-commits] [PATCH] D151003: [Damangle] convert dlangDemangle to use std::string_view

[Fangrui Song via Phabricator via lldb-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/Demangle/Demangle.cpp:49
 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) {
+  if (!MangledName)
+    return false;
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > MaskRay wrote:
> > > Why is this change? The original contract is that `MangledName` must be non-null.
> > https://fosstodon.org/@llvm/110397650834821908
> > 
> > It's insidious; implicitly constructing a `std::string_view` from a `char*` which is `nullptr` invokes a call to `strlen` upon construction, which segfaults on my system.  Therefor, all of the `nullptr` checks need to be hoisted into the callers or stated another way exist at the boundary of `char*` to `std::string_view` API boundaries (such as right here).
> This is also why the `nullptr` input test case must be removed from llvm/unittests/Demangle/DLangDemangleTest.cpp below.
It's usually code smell if a function taking a C string argument additionally accepts nullptr. 
Previously, passing `nullptr` to `nonMicrosoftDemangle` will crash as `MangledName[0]` is accessed. We should retain this strictness.

`ninja check-llvm-demangle` passes if I remove the 2 lines.

If future refactoring will possibly pass `nullptr` to `nonMicrosoftDemangle`, I think we should fix those call sites not to do that...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151003