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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


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


================
Comment at: lib/Demangle/Demangle.cpp:23
+  char *Demangled = nullptr;
+  if (strncmp(MangledName, "_Z", 2) == 0)
+    Demangled = itaniumDemangle(MangledName, Demangled, nullptr, nullptr);
----------------
erik.pilkington wrote:
> You should probably also send strings that start with "___Z" to the itanium demangler.
This patch just moves the existent code, I would not add any additional functionality in it.


================
Comment at: lib/Demangle/Demangle.cpp:26
+  else
+    Demangled = microsoftDemangle(MangledName, Demangled, nullptr, nullptr);
+
----------------
erik.pilkington wrote:
> nit: may as well just pass in `nullptr` instead of `Demangled`.
The same.


================
Comment at: tools/llvm-objdump/llvm-objdump.cpp:530
+      if (Demangle) {
+        assert(SymName->end() < StrTab.end() && *SymName->end() == '\0');
+        Target = demangle(SymName->data());
----------------
Not sure what is the point to have this assert either. I would remove it. If you want to add an additional check like this, it should be something that would report an error in release build then I think. 


Repository:
  rL LLVM

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

https://reviews.llvm.org/D56721