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

[James Henderson via Phabricator via llvm-commits]

jhenderson marked an inline comment as done.
jhenderson added a comment.

In D56721#1359558 <https://reviews.llvm.org/D56721#1359558>, @labath wrote:

> > But this does not protect us from anything in release builds.
>
> Isn't that the whole point of asserts? "Catch the things that are not supposed to happen in debug builds so that you don't have to pay the cost of checking them in release builds."


Exactly. The code isn't supposed to get into this state in the first place (it would be a programmer error if it did), so we can assume it isn't in our release builds.



================
Comment at: lib/Demangle/Demangle.cpp:20
+std::string demangle(char const *MangledName) {
+  assert (MangledName != nullptr);
+
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56721