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

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

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.



================
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
----------------
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.


================
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
----------------
Nit: please remember to end comments with a '.'.


================
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))
----------------
I don't think you need this additional complexity. `is_separator` uses `native` style by default, which should be sufficient for our purposes.


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

https://reviews.llvm.org/D85024