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

Pretty printing for closures - much better. #3179

Closed
wants to merge 1 commit into from

Conversation

dbp
Copy link
Contributor

@dbp dbp commented Aug 11, 2012

The way the pretty printer currently handles closures is distinctly unsatisfying (the elipsis are to indicate that there is enough content so that the entire expression cannot fit on a single line; if it could, things would look fine):

fn main() {
    do foo(bar) |a|
                    {
                        baa...z;
                    }
    for foo(bar) |a|
                    {
                        baz;
                        ...
                        baz;
                    }
    let f =
        |a|
            {
                foo;
                ...
                foo;
            };
}

What is happening is that the entire closure is wrapped in a pretty printing box; since it can't fit on the line with the do / for, it starts printing it and then has to break the line, and it indents relative to that, so the block is 4 spaces in from the beginning of the closure. A lot better, in my opinion (and how I think a person would write it), is:

fn main() {
    do foo(bar) |a| {
        baa...z;
    }
    for foo(bar) |a| {
        baz;
        ...
        baz;
    }
    let f = |a| {
        foo;
        ...
        foo;
    };
}

That is, the indentation is keyed on the beginning of the expression. Ideally, the way the boxes would be arranged is that the closure argument would be absorbed into the "head" box that has the do/for/let, but writing the code that way seemed to involve more code complexity, as we need to be looking for closures before we actually see the AST expression for them.

What I did, and this pull request implements, fixes the formatting for do and for expressions (they look exactly like the "desired" output). It does this by eliminating the surrounding block (which involved a little bit of refactoring to allow you to close brackets without having a surrounding box to close). It does not fix the let expression - the arguments for the closure start on the following line. I'm actually not sure why this is happening; it might be something with an easy fix. The output of the code that this pull request adds is the following:

fn main() {
    do foo(bar) |a| {
        baa...z;
    }
    for foo(bar) |a| {
        baz;
        ...
        baz;
    }
    let f = 
        |a| {
        foo;
        ...
        foo;
    };
}

I'm not sure if this is the best approach - this is the first that I've dealt with the AST or the pretty printer, so I could be doing something non-optimally. Hopefully people can given feedback on the code / how it works.

The diff includes some changes to libsyntax/print/pp.rs to make the debug output more verbose - this was helpful to me as someone unfamiliar with the code, and so seem to make sense. The rest is changes to libsyntax/print/pprust.rs, based on the changes described.

@brson
Copy link
Contributor

brson commented Aug 11, 2012

Thanks, dbp! I'm glad you are giving the pretty-printer some love. I read the patch and it is a shame the amount of gymnastics necessary to make this work. Unfortunately the pretty-printer is a bit of a mystery and when I need to update it I just hack at it until it 'works', so I can't offer any useful advice.

Rebased and pushed to incoming.

@brson brson closed this Aug 11, 2012
RalfJung pushed a commit to RalfJung/rust that referenced this pull request Mar 25, 2024
Add libc direct test for shims and update CONTRIBUTING.md

Add more ``libc`` direct test for shims as mentioned in rust-lang#3179.

Changes:
- Added ``libc`` direct tests for ``rename`` and ``ftruncate64``
- Added the command for running ``pass-dep`` test in ``CONTRIBUTING.md``
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.

2 participants