Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Unicode line drawing characters for spans #28902

Closed
wants to merge 3 commits into from

Conversation

LeoTestard
Copy link
Contributor

See #24387.

This is a port of the first PR (#21406) to the current master branch. I also added a commit to change the characters to use WGL4 instead, as suggested in the comment thread of the original PR, since those are more widely supported.

The proposal that was more or less accepted was to remove the --color and --drawing options and replace them with environment variables and a single --output-plain-ascii option. The problem is that --color still exists, and a stable option. I left --drawing for now, for consistency with --color but there are maybe better options.

This PR is not yet ready to be merged but I wanted to post it to have feedback on this issue. The proposal also included auto-enabling Unicode output when writing to a terminal and LC_CTYPE ends with .UTF-8. I'll implement this if there is still agreement that this should be done.

cc @Manishearth.

(PS: I did not change the author of the commits since I did not implement the Unicode outputting itself, but the commits from the original PR needed a lot of fixes to build on the current master branch, maybe I should have. Let me know if I should. Doing a separate commit was also possible but generated a huge diff of duplicate modifications.)

Keegan McAllister added 2 commits October 8, 2015 00:54
With some other cleanup at the use sites.
…utput

I bet we can find other uses for these sorts of characters in clarifying type
errors, etc.

This is marked as a "stable" flag for consistency with --color. However we
should reconsider both of them as part of rust-lang#19051. For these features to be
conveniently available with Cargo, we may want to use environment variables or
a config file.
@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@Manishearth
Copy link
Member

(Rather busy right now, might not get time to review, sorry. @alexcrichton, could you have a look through this?)

@alexcrichton
Copy link
Member

Thanks for picking this up @LeoTestard! I think there's definitely an interesting question of defaults here, so it's something I wouldn't mind exploring a bit.

I would be a little wary to turn this on by default as I haven't seen much precedent for CLI tools using unicode symbols by default, but on the other hand I feel like if it's turned off by default it may just end up being dead code in the compiler. For example Cargo wouldn't have an easy method of passing this down to the compiler so it'd probably end up happening that the vast majority of compiler invocations don't pass this option.

So along those lines, this seems cool, but I'm a little wary about landing this for now. cc @rust-lang/tools, though, other opinions?

@wthrowe
Copy link
Contributor

wthrowe commented Oct 9, 2015

gcc uses unicode (smart quotes) by default in a UTF-8 locale since at least version 4.6.1 (earliest I have handy to test):

$ echo 'int main() { foo x; }' | LC_ALL=en_US.UTF-8 gcc -x c - 2>&1 | od -t x1 -c
0000000 3c 73 74 64 69 6e 3e 3a 20 49 6e 20 66 75 6e 63
          <   s   t   d   i   n   >   :       I   n       f   u   n   c
0000020 74 69 6f 6e 20 e2 80 98 6d 61 69 6e e2 80 99 3a
          t   i   o   n     342 200 230   m   a   i   n 342 200 231   :
0000040 0a 3c 73 74 64 69 6e 3e 3a 31 3a 31 34 3a 20 65
         \n   <   s   t   d   i   n   >   :   1   :   1   4   :       e
0000060 72 72 6f 72 3a 20 75 6e 6b 6e 6f 77 6e 20 74 79
          r   r   o   r   :       u   n   k   n   o   w   n       t   y
0000100 70 65 20 6e 61 6d 65 20 e2 80 98 66 6f 6f e2 80
          p   e       n   a   m   e     342 200 230   f   o   o 342 200
0000120 99 0a
        231  \n
0000122

@LeoTestard
Copy link
Contributor Author

@alexcrichton I agree. We have several options here. As @wthrowe and other people said, GCC does use Unicode output by default when the locale includes UTF-8. An other option would be an environment variable, as suggested in the previous thread. This way the user can specify that it wants the Unicode output to be enabled without Cargo having to know anything about it. A third possibility is a global configuration file for Cargo in which it is possible to specify this kind of things. Both those options allow not to set Unicode output as the default in the compiler.

@alexcrichton
Copy link
Member

Aha fascinating! That seems like a plausible vector do discover whether this should be turned on by default, and then an environment variable to force it off seems plausible.

@retep998
Copy link
Member

Note that for the Windows console, the console encoding doesn't matter since we use the wide functions anyway. What really matters is the current console font, which we can get the name of via GetCurrentConsoleFontEx and then for font names that we know have the line drawing characters we can enable it by default for those. For outputting to a pipe, I'm not sure what a sensible default is.

@nrc
Copy link
Member

nrc commented Oct 12, 2015

Opinion: I'd like to see this, but I also worry about the defaults. I'd prefer to be implicit if we can, rather than use command line options or a config file. If we can be sure the user can handle the unicode lines, we should do it. But we should never do it if we can't. Can we ensure that? Or am I dreaming?

In the future, errors for tools will be serialised data structures, rather than text, so there is no issue in that context.

@sanxiyn
Copy link
Member

sanxiyn commented Oct 15, 2015

Travis failure is legit (failed tidy).

@bors
Copy link
Contributor

bors commented Nov 3, 2015

☔ The latest upstream changes (presumably #29285) made this pull request unmergeable. Please resolve the merge conflicts.

@sanxiyn
Copy link
Member

sanxiyn commented Nov 25, 2015

Do you mind if I take this over?

@alexcrichton
Copy link
Member

Closing due to inactivity, but feel free to resubmit with a rebase!

@LeoTestard
Copy link
Contributor Author

@sanxiyn I'd still like to work on this PR, but I've got the impression that there is no general consensus about what to do for the options and the defaults.

@Manishearth
Copy link
Member

@nrc now that we're in the future with proper json errors, should we push this again?

@nrc
Copy link
Member

nrc commented Mar 14, 2016

Yeah, I guess we can. I think what to do by default is the big question here, but it sounds like checking the locale is an acceptable way forward. So, if that works, let's do it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants