Uh oh!
There was an error while loading. Please reload this page.
Ergonomic format_args! - #33642
Conversation
rust-highfive
commented
May 14, 2016
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
This caused some small regressions here and there. I'll be slowly fixing them; compilation time is very long. Non-regressions, just TODO:
Noticed regressions:
|
Implemented correct implicit args handling by resolving all EDIT: Whoops, |
adf0be0 to
ca2c003Compare…tsakis syntax_ext: format: nest_level's are no more Just noticed this while working on rust-lang#33642 and here's a quick fix, shouldn't touch anything else. It's some historic code indeed...
aturon
commented
May 17, 2016
…tsakis syntax_ext: format: nest_level's are no more Just noticed this while working on rust-lang#33642 and here's a quick fix, shouldn't touch anything else. It's some historic code indeed...
alexcrichton
commented
May 20, 2016
Thanks for the PR! I'll take a closer look at this once rust-lang/rfcs#1618 is handled |
93b7b3f to
9614a17CompareFixed test cases broken by this PR, and added new cases as well. Currently
I'm not sure why those keep popping up, it wasn't the case several days ago. Testing master branch right now, will edit this once finished. Update: Another update: I recompiled without |
xen0n
commented
May 22, 2016
Travis test completed without the unrelated failures, seems there is a pretty-print round-trip failure: --- pretty.expected.rs 2016-05-22 13:39:14.080831211 +0800+++ pretty.actual.rs 2016-05-22 13:38:56.912831929 +0800@@ -178,7 +178,6 @@
, b = "hijklmn" , c = 3 ) , "abcd hijk 4\nabc hij 3");
t!(format ! ( "{a:.*} {0} {:.*}" , 4 , 3 , "efgh" , a = "abcdef" ) ,
"abcd 4 efg");
-
// Test that pointers don't get truncated.
{
let val = usize::MAX;Apart from this and deduplication of argument objects, the feature feels almost ready. Waiting for RFC resolution 😄 |
alexcrichton
commented
May 23, 2016
Ah yeah unfortunately we've had bad luck with debuginfo and LTO in the past (lots of weird LLVM assertions), but thanks for the progress update! |
I immediately gave up fixing the pretty print issue after trying to produce a minimal test case and got this: macro_rules! t {($a:expr, $b:expr) => { assert_eq!($a, $b)}}pubfnmain(){t!(format!("{:x}{:X}{:x}{:X}{:x}{a:X}",1,2,3,4,5,a=15),"abcdefghijkl");// every time this code gets pretty-printed, a newline is eaten!}Might as well reduce the length of affected lines, as the bug seems to only occur when a line wrap is needed in the output. I'll finish the argument object de-duplication and then we can start reviewing. |
b96da3d to
68dd135Comparexen0n
commented
Jun 2, 2016
Rebased and cleaned up a little, should be ready for review! PS: The reason I think an RFC is really needed, is that some serious documentation changes and maybe deprecation inside the formatting runtime will have to be done. However as the RFC is already submitted and waiting through its FCP this point is already moot. |
dea6bfd to
7037411CompareThere was a problem hiding this comment.
Can you add a test here as well with side effects on the evaluated expression to ensure it's only calculated once even though it may be formatted multiple times.
bors
commented
Jul 4, 2016
☔ The latest upstream changes (presumably #34638) made this pull request unmergeable. Please resolve the merge conflicts. |
77c4bf1 to
6d819f4Comparexen0n
commented
Jul 4, 2016
Rebased again and (finally found time to do so!) added comments to facilitate better understanding. Hope this is not too late for 1.10! |
alexcrichton
commented
Jul 13, 2016
Oops, sorry this fell off the radar! I think this got lost in my inbox by accident... Ok, everything looks good to me, thanks @xen0n! Could you also squash the commits down? Other than that r=me |
format: beautifully get rid of ArgumentNext and CountIsNextParam Now that CountIsNextParam and ArgumentNext are resolved during parse, the need for handling them outside of libfmt_macros is obviated. Note: *one* instance of implicit reference handling still remains, and that's for implementing `all_args_simple`. It's trivial enough though, so in this case it may be tolerable.
Converts named argument references into indices, right after verification as suggested by @alexcrichton. This drastically simplifies the whole process!
This commit removed the restriction of only allowing one type per argument. This is achieved by adding mappings between macro arguments and format placeholders, then taking the mapping into consideration when emitting the Arguments expression. syntax_ext: format: fix implicit positional arguments syntax_ext: format: don't panic if no args given for implicit positional args Check the list lengths before use. Fixes regression of `compile-fail/macro-backtrace-println.rs`. syntax_ext: format: also map CountIsParam indices to expanded args syntax_ext: format: fix ICE in case of malformed format args
format: workaround pretty-printer to pass tests
466558e to
51e54e5CompareSquashed all review-generated commits into respective commits and rebased again. Also clarified a few (Edit: As for missing 1.10, I was also taking a break from Rust recently so I don't really mind. 😈 ) |
alexcrichton
commented
Jul 13, 2016
bors
commented
Jul 13, 2016
⌛ Testing commit 51e54e5 with merge db71987... |
Ergonomic format_args! Fixes#9456 (at last). Not a ground-up rewrite of the existing machinery, but more like an added intermediary layer between macro arguments and format placeholders. This is now implementing Rust RFC 1618!
bors
commented
Jul 13, 2016
…crichton Update std::fmt module docs for landing of rust-lang#33642. Since rust-lang#33642, it's no longer true that all references to a given format argument must use the same type. The docs don't seem to have been updated.
…crichton Update std::fmt module docs for landing of rust-lang#33642. Since rust-lang#33642, it's no longer true that all references to a given format argument must use the same type. The docs don't seem to have been updated.
Fixes#9456 (at last).
Not a ground-up rewrite of the existing machinery, but more like an added intermediary layer between macro arguments and format placeholders. This is now implementing Rust RFC 1618!