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

Cleanup "Convert control statement bodies to block" joins multiple consecutive comments into the block #2067

Open
laeubi opened this issue Mar 9, 2025 · 3 comments

Comments

@laeubi
Copy link
Contributor

laeubi commented Mar 9, 2025

I found one case now from a review perspective it looks wrong:

https://github.com/eclipse-equinox/equinox/pull/796/files#diff-ebd60c200dde5c694d76b9ad0a08faa03db7b83c922cbc5666efe320d6099f5dR1350

Image

Basically if a comment is following the line that is wrapped inside a block it is put inside the block.

I think one should only move comments before the statement into the block (or directly at the line of the statement) but not after that line.

FYI @jjohnstn

@laeubi
Copy link
Contributor Author

laeubi commented Mar 9, 2025

Here is an example where the comments are before the statement what looks correct:

https://github.com/eclipse-equinox/equinox/pull/796/files#diff-75f843fba67bbdbdf3a2879721b7f48cefc84fdb2d856233b13ae29fcf4a157cR752

Image

in this case I think the first comment could be on the next line, but if it complicated things too much it is not mandatory I think it just seems it could improve readability of the code.

@jjohnstn
Copy link
Contributor

@laeubi The comment gets connected to the return statement by the compiler and the cleanup doesn't actually look at it or manipulate. It simply copies the return statement (extended range which includes its comments) into the block via ASTRewrite. The rationale is that if you already had a block with such a comment at the end and were to make a new block and copy all the statements, you would want such a trailing comment to come along automatically and not require special processing. to find it. I think a new range computer is required to allow the caller to specify extended range but not to bring along comments that follow the line of the statement. I'll have a look at it.

@laeubi
Copy link
Contributor Author

laeubi commented Mar 10, 2025

Yes I already guessed it is not trivial, in this particular example you can only see by the indentation (what of course means nothing to the compiler) that the second comment does not belong to the first one.

When it is too complex I would say it is a corner case we most likely can not fix in an automated way and then should be fixed manually.

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

No branches or pull requests

2 participants