Skip to content

uncrustify: split lines at 80 chars#10873

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
kaspar030:limit_uncrustify_line_length
Mar 7, 2019
Merged

uncrustify: split lines at 80 chars#10873
miri64 merged 1 commit intoRIOT-OS:masterfrom
kaspar030:limit_uncrustify_line_length

Conversation

@kaspar030
Copy link
Copy Markdown
Contributor

Contribution description

Currently, uncrustify doesn't care about too long lines. Unfortunately it also creates them itself, see this.

This PR configures uncrustify to allow a maximum of 100 chars per line.

Testing procedure

Check uncrustify output with and without this change.

Issues/PRs references

#10867

@kaspar030 kaspar030 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: tools Area: Supplementary tools labels Jan 25, 2019
@kaspar030 kaspar030 requested review from jia200x and miri64 January 25, 2019 21:30
@kaspar030 kaspar030 mentioned this pull request Jan 25, 2019
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 26, 2019

Needs rebase.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 26, 2019

Here is what I dread: If you want to make uncrustify the authoritative decision maker but with this change you basically break

  • Line length: aim for no more than 80 characters per line, the absolute maximum should be 100 characters per line.

from our coding conventions. I always (at least for the last few years... don't look at my old code you monster!) interpreted this as 80 chars is desirable, but if you absolutely have to you can go up to hundred (e.g. with a break at 80 it would be less readable or URLs in doc [though those might actually overreach 100]). So I would say: set it to 80 and if there are instances where 100 is needed use begin{code-style-ignore} (unless there is a way to get uncrustify to interpret this as it is written...).

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 28, 2019

So I would say: set it to 80 and if there are instances where 100 is needed use begin{code-style-ignore} (unless there is a way to get uncrustify to interpret this as it is written...).

+1

I just tried it with code_width=80 and looks fine :)

@kaspar030
Copy link
Copy Markdown
Contributor Author

Wrapping in the begin/end markers for function declarations? Nah that looks weird. ;)

I just tried it with code_width=80 and looks fine :)

Hm, I'm not so sure. The coding conventions say "desirable" for a reason, it doesn't make sense to force it.

With uncrustify's current config, it would try hard to keep e.g., "int foobar" together. If set to 80 and "foobar" crosses that, uncrustify would wrap before the "int". IMO it looks better to have foobar cross over the 80 chars in those cases. But "desirable" (compared to "forced") is not being argued here, right?

Setting uncrustify to 100 max line length would do what the coding conventions currently express: allow some lines to be somewhat longer in order to prevent ugly wraps, at the discretion of the author. (Also, don't let uncrustify create overlength lines itself, as happened here.)

I'd like to arrive at an uncrustify config that becomes "fire and forget". If it constantly shortens my declarations because of a couple of chars over 80, I personally find it too intrusive.

May I suggest staying with 100 chars (as in, lines really shouldn't be longer), and let the extra characters be leeway as per coding conventions?

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Jan 28, 2019

May I suggest staying with 100 chars (as in, lines really shouldn't be longer), and let the extra characters be leeway as per coding conventions?

Do you know if Uncrustify enforces ALL lines to be lower than 100 in this case?

If so, I'm OK with the proposal ;)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 28, 2019

I'd like to arrive at an uncrustify config that becomes "fire and forget". If it constantly shortens my declarations because of a couple of chars over 80, I personally find it too intrusive.

See, this is where I see the problem. If we argue that cppcheck is fire and forget, we forget that our actual line length is 80 chars. ;-)

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 28, 2019

Or alternatively we get the style discussions you dread, because a maintainer wants a line to be <80, but uncrustify gave you a longer line ;-).

@kaspar030
Copy link
Copy Markdown
Contributor Author

Ok, in the name of not having to discuss this, this now sets 80 chars line limit.

@kaspar030 kaspar030 force-pushed the limit_uncrustify_line_length branch from e61ff2d to f26425f Compare March 4, 2019 15:15
@kaspar030 kaspar030 changed the title uncrustify: split lines at 100 chars uncrustify: split lines at 80 chars Mar 6, 2019
@kaspar030
Copy link
Copy Markdown
Contributor Author

@miri64 wanna take another look?

@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 7, 2019
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

ACK. Though this might yield some false positives in the CI if we use uncrustify there, but I think we could have this solved by using different tools there.

@miri64 miri64 merged commit e288c9b into RIOT-OS:master Mar 7, 2019
@danpetry danpetry added this to the Release 2019.04 milestone Mar 11, 2019
@kaspar030 kaspar030 deleted the limit_uncrustify_line_length branch July 5, 2019 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants