[PATCH] D113106: demangle xcoff label symbol for llvm-nm

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

Similar to the comment on the size patch, I think you want a drastically simpler test, which has just enough symbols to exercise the new demangling code (e.g. something that doesn't get demangled, something that has a dot, something that doesn't have a dot, and something that has an empty name). At the moment, it is hard to spot what's actually important in the test.



================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:620-621
 
-static Optional<std::string> demangle(StringRef Name, bool StripUnderscore) {
+static Optional<std::string> demangle(StringRef Name, bool StripUnderscore,
+                                      bool isXCOFF) {
   if (StripUnderscore && !Name.empty() && Name[0] == '_')
----------------
Rather than have all these boolean parameters to change the behaviour of the function, I think you probably want separate `demangleMachO` and `demangleXCOFF` functions. They would do the stuff specific to their format, before calling the common demangle function.

Rough outline:
```
static Optional<std::string> demangleMachO(StringRef Name) {
  // if name not empty, drop front of Name.
  return demangle(Name);
}

static Optional<std::string> demangleXCOFF(StringRef Name) {
  bool HasDot = /* ... */;
  // if HasDot and name not empty, drop front of Name.
  Optional<std::string> Demangled = demangle(Name);
  if (!Demangled)
    return None;
  return "." + *Demangled;
}

static Optional<std::string> demangle(StringRef Name) {
  // common stuff
}

// When calling...
    MachOObjectFile *MachO = dyn_cast<MachOObjectFile>(&Obj);
    if (Demangle) {
      function_ref<Optional<std::string> DemangleFn;
      if (MachO)
        DemangleFn = demangleMachO;
      else if(dyn_cast<XCOFFObejctFile>(&Obj))
        DemangleFn = demangleXCOFF;
      else
        DemangleFn = demangle;
      if (Optional<std::string> Opt = DemangleFn(S.Name)
        Name = *Opt;
    }
```


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:627
+  if (isXCOFF && !Name.empty() && Name[0] == '.') {
+    Name = Name.substr(1);
+    isPutXCOFFLabelDotBack = true;
----------------
Consider using `StringRef::drop_front` instead, as it's a little more descriptive - probably only if you agree with my suggested refactor above (and then do it for the Mach-O case too).


================
Comment at: llvm/tools/llvm-nm/llvm-nm.cpp:642
+  if (isPutXCOFFLabelDotBack)
+    S.insert(0, 1, '.');
+
----------------
`S = "." + S;` seems much clearer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D113106