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

[Vinicius Tinti via Phabricator via llvm-commits]

tinti marked 5 inline comments as done.
tinti added inline comments.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test:51
+; RUN:   FileCheck %s --check-prefix=CHECK-MISSING-PREFIX-FIX
+
----------------
jhenderson wrote:
> 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.
> 
I agree.


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


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:1042
+      int Level = 0;
+      const char *FileName = LineInfo.FileName.c_str();
+
----------------
jhenderson wrote:
> Perhaps worth an assert that shows that `LineInfo.FileName` is not empty.
Yes. I will place it just after `is_absolute_gnu`.


================
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;
+        }
+      }
----------------
jhenderson wrote:
> 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.
Yes I have. But it skips extra separators which is not what GNU does.

https://github.com/llvm/llvm-project/blob/004a264f8c923922ecd34255bcb10f4adaa27ac5/llvm/lib/Support/Path.cpp#L267

To make '/foo//bar/baz' become '/baz' --prefix-strip must be 3.

I will add a test for it too.


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

https://reviews.llvm.org/D96679