[PATCH] D56721: Move llvm-objdump demangle function into demangler library

[Erik Pilkington via Phabricator via llvm-commits]

erik.pilkington added inline comments.


================
Comment at: lib/Demangle/Demangle.cpp:20
+std::string demangle(char const *MangledName) {
+  assert (MangledName != nullptr);
+
----------------
jhenderson wrote:
> grimar wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > grimar wrote:
> > > > > This line does not seem to be clang-formatted, but I also think it is not useful.
> > > > > In case MangledName is null, you'll have a crash anyways, the crash is an obvious assert by itself.
> > > > > From what I saw in LLVM's code, assert is more commonly used to check the more complex logic flows, so I would remove it. 
> > > > This protects us against undefined behaviour in strncmp below. It may not be a crash if nullptr is passed in.
> > > But this does not protect us from anything in release builds.
> > > Should it be
> > > 
> > > ```
> > > if (!MangledName)
> > >   llvm_unreachable(...);
> > > ```
> > > 
> > > then?
> > Seems you can just change the argument to `const std::string&`?
> I'd rather not do that, if I can avoid it. If the user already knows their string is a null-terminated string (e.g. because it's a string literal), they can just pass in the relevant pointer, without being forced to incur the cost of copying to a std::string. In other situations, I'd use a `StringRef`, to avoid the copy, but that would introduce a dependency on the Support library, which we can't do here.
For the record, I would also prefer doing `const string &` parameter. The cost of copying the demangled string is already going to be much more than the input string, so I don't think the performance argument is that great. Your call though.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56721