[PATCH] D96679: [llvm-objdump] Implement --prefix-strip option

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:178
+  When disassembling with the :option:`--source` option, strip out ``level``
+  initial directories from absolute paths. It has no effect without
+  :option:`--prefix`.
----------------
Same in the man page.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test:67
+
+;; Test --prefix-strip value equal to the number of directory components.
+
----------------
I think it would be worth making this and the following cases positive cases (i.e. where the behaviour results in a valid source interleave), if possible. Also, I think adding to this comment what the path before and after the --prefix-strip would be, as it makes it easier to envision what is going on.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test:72
+
+;; Test --prefix-strip value greater than the number of components.
+; RUN: llvm-objdump --prefix %p --prefix-strip 2 --source %t-missing-prefix.o 2>&1 | \
----------------
Perhaps also worth testing a negative value to show that llvm-objdump reports an error in this case.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:364
+                         cl::desc("Strip out initial directories from absolute "
+                                  "paths. It has no effect without --prefix"),
+                         cl::init(0), cl::cat(ObjdumpCat));
----------------



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1045-1054
+      const char *FileName = LineInfo.FileName.c_str();
+
+      // Path.h iterator skips extra separators. Therefore it cannot be used
+      // here to keep compatibility with GNU Objdump.
+      for (const char *S = FileName + 1; *S != '\0' && Level < PrefixStrip;
+           ++S) {
+        if (sys::path::is_separator(*S)) {
----------------
I think you can avoid converting to a C-string here, by doing something like the following, if I'm not mistaken:
```
auto StrippedNameStart = LineInfo.FileName.begin();
for (auto Pos = LineInfo.FileName.begin() + 1, End = LineInfo.FileName.end(); Pos != End && Level < PrefixStrip; ++Pos) {
  if (sys::path::is_separator(*Pos)) {
    StrippedNameStart = Pos;
    ++Level;
  }
}
LineInfo.FileName = std::string(Post, LineInfo.FileName.end());
```


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

https://reviews.llvm.org/D96679