[PATCH] D70720: [llvm-objdump] Display locations of variables alongside disassembly

Thu Feb 20 01:06:35 PST 2020

jhenderson added inline comments.

================
Comment at: llvm/docs/CommandGuide/llvm-objdump.rst:132-135
+.. option:: --debug-vars-unicode=<true|false>
+
+  If true, use unicode characters when displaying source-level variables. If
+  false, use only ASCII characters. Defaults to true.
----------------
echristo wrote:
> jhenderson wrote:
> > ostannard wrote:
> > > jhenderson wrote:
> > > > This isn't quite the interface I'd use. I'd actually recommend making ASCII the default, since it will work in all situations, rather than requiring Windows users to specify an extra switch. If there's a good reason not to, I'd change this to --debug-vars-ascii, so that it's the normal practice of "use a switch to enable a behaviour" i.e. `--debug-vars-unicode` is all that is needed (and there's no need to do the slightly weird syntax of `--debug-vars-unicode=false`).
> > > > 
> > > > A third option (possibly in conjunction with the third) would be to change the switch name to --debug-vars-style, and allow it to take an enum of ASCII/Unicode (or more specifically UTF-8 probably). However, I don't think it's likely that we'll ever need more than two modes, so this is probably overkill.
> > > > 
> > > > My personal preference would be the first of the option, as I am primarily Windows developer, as are all of the developers I work with/make tools for.
> > > I've tested this on windows 10, and the unicode output displays correctly (in both cmd.exe and powershell). Is there some other version or configuration of windows in which this won't work? If ASCII is only needed for older versions of windows, I'd like to keep the default as unicode, because I find it much more legible than the ASCII version. There's also the option of making ASCII the default for windows and unicode elsewhere, but I'd rather avoid that if possible.
> > > 
> > > I do like the idea of switching the spelling to `--debug-vars-ascii` though.
> > Thanks. Could you remove the "=<true|false>" bit from the description (you don't include it for --debug-vars after all...). Also replace "If true" with "If specified" and ". If false, use" with ", otherwise, use" and delete the "Defaults to false" sentence.
> FWIW I'd have probably gone with --debug-vars=<encoding> ?
> FWIW I'd have probably gone with --debug-vars=<encoding> ?

I like this idea. Saves the need for two switches, and makes things nice and explicit about what encoding to use.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70720