libterm: port changes from term, refactor unwraps() into pattern matches #29999
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
|
This doesn't remove all usages of unwap(), I've left in some where I think using pattern matching doesn't simplify the code, or would make it more verbose (extra indentation levels, for example). Please advise as to if further instances should be removed. |
|
This looks like a very recurring pattern. Maybe all single pop and dual pop arms should simply call into another function where they are matched on again, but the single/dual pop is done once and the values can be used directly in the arms. |
|
This is originally the same code as term but they are drifting apart. It's unfortunate; this is an example of double work since term has already merged a change called “avoid unwrap”. |
|
Interesting, I was unaware of where this originated. I'll rework this to use the same format as term. |
|
Cheers @oli-obk and @bluss, I've updated this PR to be more in line with the changes made to term, which should also cut down on much of the duplication that was present. I've left in a TODO (now a FIXME) from the term code on line 122, as it appears there should be some bound checking here (please correct me if not), but that seems outside the scope of this. |
|
@SingingTree, there (probably) shouldn't be any bounds checking as ncurses doesn't have any (although, to be fair, ncurses doesn't do much type checking of variables at all...). |
|
Also, I recommend you just import all of term as is as I've made a few bug fixes (most importantly, the format string code now obeys the spec). |
|
Sure thing, I will amend this to reflect changes made to term. I was unaware of the status of libterm and term (for anyone reading this who isn't aware, these were useful refs for me: #26042, https://internals.rust-lang.org/t/submodules-in-rust-lang-rust-for-external-repositories/2200). |
|
I've been messing around with bringing changes across, however I see that win.rs now uses the winapi and kernel32 crates. Let me know if I'm incorrect, but my thinking is that since these aren't part of the rust src, I can't use them as deps in libterm. Is this correct? I'm continuing on with the refactor, which means I've had to gut some of the new functionality. Sound like a reasonable course of action? |
|
11a1fa6 reflects my current efforts at bringing across the term changes. I'd like to have another look at it to make sure I haven't done anything silly, so consider it currently a WIP. |
SingingTree
changed the title
|
This seems like it's steadily growing in scope over time, so I'm curious to take a step back for a second with a few questions:
I hope to soon have a branch to at least start out building the compiler with Cargo at which point integration with the upstream libterm should be much easier, so I'm a little wary to put a lot of effort into the internal libterm if it's largely just refactorings (e.g. just a bunch of extra churn) |
|
It did indeed start as refactoring to remove unwraps, similar to the changes made here Stebalien/term@a82086b. Based on @Stebalien comment to bring across some of the other fixes made in term this has grown in scope. This should bring in a number of other changes and fixes from term such as those in Stebalien/term@c62def5 and Stebalien/term@031b0eb. Nothing from this should go upstream to term, it should all be alterations already made in term. There is some ugliness in terms of not all of the changes being able to be brought across (due to cargo deps; mainly the Terminal trait hasn't got all its new functionality), so this is a combination of the old libterm and new changes from term. |
|
Ok, in that case this is probably fine to merge for now and we can deal with using an upstream version later on. In the meantime this doesn't set us backwards either way! Could you squash the commits down into one commit? |
|
Will do. |
SingingTree
force-pushed
the
libterm_unwrapping
branch
from
2087c36
to
f4b787f
Compare
|
Seems good to me. I'm inclined to r+ but I'm not really familiar with this code. I'd love to see @Stebalien give this a once over. |
|
LGTM (I've ready the changes but haven't tested them). I noticed that |
|
Huh, I thought I got the searcher changes. I'll bring them in also and update this. |
SingingTree
force-pushed
the
libterm_unwrapping
branch
2 times, most recently
from
2b2ed80
to
6df9e39
Compare
This brings across changes made to the term library to libterm. This includes removing instances or unwrap, fixing format string handling, and removing a TODO. This fix does not bring all changes across, as term now relies on cargo deps that cannot be brought into the rust build at this stage, but has attempted as best to cross port changes not relying on this. This notably limits extra functionality since implemented int he Terminal trait in Term. This is in partly in response to rust issue rust-lang#29992.
SingingTree
force-pushed
the
libterm_unwrapping
branch
from
6df9e39
to
0ee230a
Compare
This removes a number of instances of unwrap and replaces them with pattern matching. This is in response to rust issue #29992.
Resetting the terminal should first try `sgr0` (as per the comment), not `sg0` which I believe to be a typo. This will at least fix rustc output in Emacs terminals (e.g., ansi-term) with `TERM=eterm-color` which does not provide the next fallback `sgr`. In such a terminal, the final fallback `op` (`\e[39;49`) is used which resets only colors, not all attributes. This causes all text to be printed in bold from the first string printed in bold by rustc onwards, including the terminal prompt and the output from all following commands. The typo seems to have been introduced by rust-lang#29999
… r=alexcrichton Fix default terminfo code for reset (sg0 -> sgr0) Resetting the terminal should first try `sgr0` (as per the comment), not `sg0` which I believe to be a typo. This will at least fix rustc output in Emacs terminals (e.g., ansi-term) with `TERM=eterm-color` which does not provide the next fallback `sgr`. In such a terminal, the final fallback `op` (`\e[39;49`) is used which resets only colors, not all attributes. This causes all text to be printed in bold from the first string printed in bold by rustc onwards, including the terminal prompt and the output from all following commands. The typo seems to have been introduced by rust-lang#29999
This removes a number of instances of unwrap and replaces them with
pattern matching.
This is in response to rust issue #29992.