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

Fix indent_continue not indenting nested statements #3816

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dbartolini
Copy link
Contributor

@dbartolini dbartolini commented Aug 19, 2022

Allows indent_continue to increment the indentation level in nested statements.
The LANG_OC check is to ensure previous behavior since this could break Objective-C code in strange ways I will not be able to fix properly at the moment.

This fixes all indenting problems in my C++ codebase without introducing regressions, especially in code inside lambdas, but feedback and broader testing is very much appreciated.

Fixes #3752.

Copy link
Collaborator

@micheleCTDEAdmin micheleCTDEAdmin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ciao Daniele,
I tested the PR over 3200 files and it does fix a lot of mis-indentation.
On the other hand I also spotted few regressions, with some code being indented too much. The regressions are similar in concept to the modified "expected" cpp files in this PR, where you can see extra indentation added.
If you could find a way to fix this PR so that the extra indentation is not present, it would be a great PR to merge.

@dbartolini
Copy link
Contributor Author

Assuming the regressions are similar to the one in tests/expected/cpp/31593-sf593.cpp: should indent_continue avoid incrementing the indentation when "nested statements are on the same line"?

The other modified "expected" .cpp files should not contain more indentation than necessary I think, could you confirm that?

@micheleCTDEAdmin
Copy link
Collaborator

Yes, the regression I observed are similar to the one in tests/expected/cpp/31593-sf593.cpp. If you need, I can provide some examples.

Regarding the other modified tests, it could probably be a subjective thing. For example tests/expected/cpp/60055-issue_3116.cpp, the body of the lambda function is intended twice compared to the B function. While technically there are three levels of indentation (A -> B -> lambda body) and so it would explain the extra indentation, on the other hand it does look a bit awkward. What's your take on it?

@dbartolini
Copy link
Contributor Author

Yes, the regression I observed are similar to the one in tests/expected/cpp/31593-sf593.cpp. If you need, I can provide some examples.

It would be helpful, thanks!

Regarding the other modified tests, it could probably be a subjective thing. For example tests/expected/cpp/60055-issue_3116.cpp, the body of the lambda function is intended twice compared to the B function. While technically there are three levels of indentation (A -> B -> lambda body) and so it would explain the extra indentation, on the other hand it does look a bit awkward.

It does look awkward indeed, but it is consistent with how lambdas are indented normally. In this case, the ) of B is on the same line with the } of its lambda parameter, and that is what makes it look weird, I think. Had the closing parentheses been on its own line, it would have looked nice and correct:

A(
    B([] (const std::string &s) -> bool {
            s = "hello";
            return true;
        }
    ), 1
);

Moreover, if you add extra parameters to B, with this PR you will get something like this, which I think it is correct:

A(
    B(foo,
        bar,
        [] (const std::string &s) -> bool {
            s = "hello";
            return true;
        }
    ), 1
);

Compare it with the current behavior; doesn't this look even weirder with those parameters squashed to B()'s level?

A(
    B(foo,
    bar,
    [] (const std::string &s) -> bool {
        s = "hello";
        return true;
    }
    ), 1
);

Like you said, it may be a subjective thing, and another option to adjust for personal preferences would be needed I guess.

@micheleCTDE
Copy link
Collaborator

micheleCTDE commented Aug 21, 2022

I have attached an example showing some regressions, including the config used to test it. Note that I use negative values for indent_continue.

Re lambda indentation, I see the point in your comment. Also there is an interesting idea proposed in issue #3815, so the solution may be to merge this PR (once the other regressions are fixed) and then add an option as proposed by @PoeticPete.

pr_3816.zip

@dbartolini dbartolini marked this pull request as draft August 24, 2022 19:26
@dbartolini dbartolini force-pushed the fix-nested-indent_continue branch 2 times, most recently from 6b3e58a to 2adf01c Compare November 21, 2022 13:29
@dbartolini dbartolini force-pushed the fix-nested-indent_continue branch 2 times, most recently from 6b3e58a to 2adf01c Compare April 11, 2023 16:05
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.

indent_continue breaks nested continuation indents?
3 participants