[PATCH] D76733: New symbolizer option to print files relative to the compilation directory.

[James Henderson via Phabricator via llvm-commits]

jhenderson accepted this revision.
jhenderson added a comment.

LGTM, with a nit.



================
Comment at: llvm/docs/CommandGuide/llvm-symbolizer.rst:154
+full path is /tmp/foo/test.cpp and is compiled as follows. The first case
+shows the default absolute path, the second --basenames and the third
+shows --relativenames.
----------------
I think, though I suspect opinions may vary, there should be a comma between "--basenames" and "and", to delimit the list properly.


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:80
 
+// -relativenames
+static cl::opt<bool>
----------------
MaskRay wrote:
> saugustine wrote:
> > MaskRay wrote:
> > > Double-dashed `--relativenames` may be better (and is the one in the documentation and --help output)
> > The code as written allows one to specify it either way. Just like basenames.
> Yes, this is how llvm::cl::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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76733