New "--relative" option to allow printing files relative to the compilation directory.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
This change looks good to me. My mere concern is about the option naming. In GNU addr2line, -r has a different meaning, and llvm-symbolizer can be used as llvm-addr2line (it is a crunched executable). If it does not cause too much inconvenience, I will hope we can make llvm-symbolizer and llvm-addr2line have consistent option names.
The clang-format recommendations would change the new options to not match the format of old ones. Not sure the best thing to do there.
What would you choose for a different name? I am open to about anything. I suppose we wouldn't even need a short option at all.
Remove short option. It only existed to be parallel to "-s", but conflicts
with options in gnu addr2line.
With all of our tools with GNU equivalents, we should avoid single letter aliases unless we can get GNU to buy into the new option too. Long names are less of an issue, as it's unlikely we'll end up with a clash.
Please add the option to the llvm-symbolizer and llvm-addr2line docs.
Could you post a quick output comparison please for with and without this option? I'm finding it hard to properly visualise the behaviour from the test.
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp | ||
---|---|---|
81 | clang-format appears to be complaining. | |
83 | No need for this comment. | |
318–323 | This "pick last" behaviour needs testing. |
llvm/docs/CommandGuide/llvm-symbolizer.rst | ||
---|---|---|
154 ↗ | (On Diff #252997) | paths -> path |
171 ↗ | (On Diff #252997) | Nit: only one blank line, please. |
llvm/test/tools/llvm-symbolizer/relativenames.s | ||
9 | Something I'm trying to encourage others to do is use '##' for comments, to distinguish them from lit and FileCheck directives. Also, please add a blank line before this comment to separate it from the regular test case. | |
13 | No need for the -DDIR option here. | |
15 | I'd explicit make this prefix "RELATIVE" to help contrast the last-one-wins cases. | |
16 | This doesn't actually show that the basenames won: by default, check patterns looks for substrings that match, not just full lines (see FileCheck --match-full-lines). You'll want to add at the start the start-of-line regex: {{^}}. |
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp | ||
---|---|---|
80 | Double-dashed --relativenames may be better (and is the one in the documentation and --help output) |
updated for all comments.
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp | ||
---|---|---|
80 | The code as written allows one to specify it either way. Just like basenames. |
The code change looks good but please wait for @jhenderson's approval.
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp | ||
---|---|---|
80 | Yes, this is how llvm:🆑:ParseCommandLineOptions works unless LongOptionsUseDoubleDash is set to true. This is not ideal, though. For references in comments/documentation we should just use the canonical form: --relativenames. We might choose to set LongOptionsUseDoubleDash for llvm-addr2line to disambiguate option parsing. |
LGTM, with a nit.
llvm/docs/CommandGuide/llvm-symbolizer.rst | ||
---|---|---|
154 ↗ | (On Diff #253739) | I think, though I suspect opinions may vary, there should be a comma between "--basenames" and "and", to delimit the list properly. |
llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp | ||
80 | I'd leave as-is in this patch, since the other options are commented in this same style. Arguably, those comments don't add anything, since the option name is listed in the code, so I'd accept a follow-up to delete the comments (and if you want to them omit it from this line in this patch, that's fine by me), or alternatively update them to use double-dashes instead. |
Something I'm trying to encourage others to do is use '##' for comments, to distinguish them from lit and FileCheck directives.
Also, please add a blank line before this comment to separate it from the regular test case.