The option --prefix-strip is only used when --prefix is not empty.
It removes N initial directories from absolute paths before adding the
prefix.
This matches GNU's objdump behavior.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test | ||
|---|---|---|
| 45 | ||
| 51 | 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:
| |
| llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
| 361 | Are you sure you want int? That implies this can take a negative number. Likely you want something like uint32_t... | |
| 1042 | Perhaps worth an assert that shows that LineInfo.FileName is not empty. | |
| 1046–1052 | 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. | |
| 3000–3001 | If you don't use int, then this will be sorted by the command-line library itself. | |
| llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test | ||
|---|---|---|
| 51 | I agree. | |
| llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
| 361 | I agree. | |
| 1042 | Yes. I will place it just after is_absolute_gnu. | |
| 1046–1052 | Yes I have. But it skips extra separators which is not what GNU does. To make '/foo//bar/baz' become '/baz' --prefix-strip must be 3. I will add a test for it too. | |
Comment Actions
Add range tests.
Add extra separator test.
Add assert.
Use uint32_t instead of int.
Comment about not using Path.h iterator.
| llvm/docs/CommandGuide/llvm-objdump.rst | ||
|---|---|---|
| 178 | Same in the man page. | |
| llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test | ||
| 67 | 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. | |
| 72 | Perhaps also worth testing a negative value to show that llvm-objdump reports an error in this case. | |
| llvm/tools/llvm-objdump/llvm-objdump.cpp | ||
| 364 | ||
| 1045–1054 | 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()); | |
Comment Actions
Reword documentation.
Use good result test instead of bad result.
Add invalid value test.
Convert C code to C++.
Comment Actions
Sorry for the delay. I've been off sick for much of the week, and am still catching up on things.
| llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test | ||
|---|---|---|
| 84 | Another one that has occurred to me: testing a non-numeric value, e.g. "--prefix-strip=foo" to show an error is hit. Not 100% certain if it's necessary, but it ensures we have "invalid case" handling in place, in case negative numbers are ever accepted for some reason. | |
Comment Actions
I hope you are well. Be safe.
| llvm/test/tools/llvm-objdump/X86/source-interleave-prefix.test | ||
|---|---|---|
| 84 | I agree. | |
Same in the man page.