nickdesaulniers added inline comments.
================ Comment at: llvm/lib/Demangle/Demangle.cpp:49 bool llvm::nonMicrosoftDemangle(const char *MangledName, std::string &Result) { + if (!MangledName) + return false; ---------------- MaskRay wrote: > 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... > We should retain this strictness. > If future refactoring will possibly pass nullptr to nonMicrosoftDemangle, I > think we should fix those call sites not to do that... Ok, I will drop it then, and add nullptr checks to callers. > ninja check-llvm-demangle passes if I remove the 2 lines. Then `check-llvm-demangle` must not be running llvm/unittests/Demangle/DLangDemangleTest.cpp, or you tested with my change to llvm/unittests/Demangle/DLangDemangleTest.cpp from below applied. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D151003/new/ https://reviews.llvm.org/D151003 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits