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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test:45
+;; Test invalid source interleave fixed by adding the correct prefix and
+;; stripping out an extra directory in path.
+
----------------



================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test:51
+; RUN:   FileCheck %s --check-prefix=CHECK-MISSING-PREFIX-FIX
+
----------------
Looks like there's one too many blank lines at the end of this file?

Also, I think you should have a few additional test cases:

  # A --prefix-strip value of 0.
  # A --prefix-strip value equal to the number of directory components.
  # A --prefix-strip value greater than the number of components.



================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:361
 
+cl::opt<int>
+    objdump::PrefixStrip("prefix-strip",
----------------
Are you sure you want `int`? That implies this can take a negative number. Likely you want something like uint32_t...


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1042
+      int Level = 0;
+      const char *FileName = LineInfo.FileName.c_str();
+
----------------
Perhaps worth an assert that shows that `LineInfo.FileName` is not empty.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1046-1052
+      for (const char *S = FileName + 1; *S != '\0' && Level < PrefixStrip;
+           ++S) {
+        if (sys::path::is_separator(*S)) {
+          FileName = S;
+          ++Level;
+        }
+      }
----------------
Have you looked at using the path component iterator functionality provided by Path.h to do this instead? It seems like that would be a preferable solution to your hand-rolled version here.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:2996-2997
 
+  if (PrefixStrip < 0)
+    reportCmdLineError("prefix strip must be non-negative");
+
----------------
If you don't use `int`, then this will be sorted by the command-line library itself.


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

https://reviews.llvm.org/D96679