[PATCH] D85024: [llvm-objdump] Implement --prefix option

[Vinicius Tinti via Phabricator via llvm-commits]

tinti added a comment.

In D85024#2237935 <https://reviews.llvm.org/D85024#2237935>, @jhenderson wrote:

> Having given it some thought, I think the right thing to do might be to put a method alongside the existing `is_absolute` method called something like either `is_rooted`, or `is_gnu_style_absolute` or similar. That function would then look something like this:
>
>   bool is_rooted(StringRef Path, Style St) {
>     if (is_absolute(Path, St))
>       return true;
>     if (real_style(St) == windows)
>       return !Path.empty() && is_separator(Path.front(), St);
>     return false;
>   }
>
> I would probably recommend that that be added in a separate patch, with dedicated gtest unit tests and reviewers taken from among those who have recently worked in this area (plus myself). You would then rebase this patch on top of that one.

Ok. I will be working on that.

I prefer using `is_gnu_style_absolute`. It makes clear that this function was created for compatibility reasons.



================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:29
+; RUN: llvm-objdump --prefix myprefix/// --source %t.o 2>&1 | FileCheck %s --check-prefix CHECK-TRAILING -DFILE=%t.o
+; CHECK-TRAILING: warning: '[[FILE]]': failed to find source myprefix/Inputs/source-interleave-x86_64.c
----------------
jhenderson wrote:
> tinti wrote:
> > jhenderson wrote:
> > > You don't need `CHECK-TRAILING`. You can use `CHECK-BROKEN-PREFIX` with `-D ROOT=''`.
> > In this case I believe I need.
> > 
> > `CHECK-BROKEN-PREFIX` uses `%t2`.
> > `CHECK-TRAILING` uses `%t`.
> `CHECK-BROKEN-PREFIX` and `CHECK-TRAILING` do NOT use `%t` or `%t2`. What do you think the `-DFILE` bit of the FileCheck invocation does?
> 
> Please take this in the well-meaning way it's intended, but I'd highly recommend you read through the FileCheck documentation at https://llvm.org/docs/CommandGuide/FileCheck.html. It seems that you may have several misunderstandings about how FileCheck works.
You can be crystal clear with me.

I red again the docs. Now it is more clear what you were saying.

There is no need to use `CHECK-TRAILING: warning: ...` because I can reuse `CHECK-BROKEN-PREFIX: warning: ...` with different parameters from `-D`.

My understanding was wrong. Given a RUN command, I thought FileCheck would only look for CHECKs after it.


================
Comment at: llvm/test/tools/llvm-objdump/X86/source-interleave-absolute-paths.test:32
+
+;; Test malformed input
+; RUN: llvm-objdump --prefix myprefix --source %t3.o 2>&1 | FileCheck %s --check-prefix CHECK-MALFORMED -DFILE=%t3.o -DROOT=%/p
----------------
jhenderson wrote:
> Nit: please remember to end comments with a '.'.
Ok.


================
Comment at: llvm/tools/llvm-objdump/llvm-objdump.cpp:3011-3014
+    bool IsOsWindows = Triple(sys::getProcessTriple()).isOSWindows();
+    while (sys::path::is_separator(Prefix.back(),
+                                   IsOsWindows ? sys::path::Style::windows
+                                               : sys::path::Style::native))
----------------
jhenderson wrote:
> I don't think you need this additional complexity. `is_separator` uses `native` style by default, which should be sufficient for our purposes.
Ok.


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

https://reviews.llvm.org/D85024