uncrustify: change en/disable markers #10865

kaspar030 · 2019-01-25T10:32:42Z

Contribution description

Default uncrustify uses *INDENT-OFF* as marker for disabling all formatting processing.
As a lot more than indenting is disabled, I find that misleading. Also it kinda screams at me using all-caps.

boards/hifive1/board.c is currently the only non-vendor file in the repository that made use of the markers. Previously, it used a speaking comment à la /* disable uncrustify *INDENT-OFF* */ .

I'd like to change the marker to speak for itself, but allows changing of the used tool.

This PR changes uncrustify's markers to *auto-format-off* and auto-format-on.

What do you think?

Testing procedure

See uncrustify output for boards/hifive1/board.c with / without PR.

Issues/PRs references

none

jia200x · 2019-01-25T10:48:37Z

+1 to the change to uncrustify markers

miri64

I'm aware this is very much a question of taste, but

why keep the asterisks?
why not a more environment-like thing. It feels unintuitive to me that to enter a certain region of code you say "off" and to leave it you say "on". I'd rather expect something like
```
/* begin{code-style-ignore} */
some* dirty=code;
/* end{code-style-ignore} */
```
(totally inspired by Latex ;P).

kaspar030 · 2019-01-25T11:39:32Z

2\. It feels unintuitive to me that to enter a certain region of code you say "off" and to leave it you say "on".

Yeah, that captures my feelings, too. Are we fine with having a begin but no end, e.g., when disabling uncrustify for the whole file?

miri64 · 2019-01-25T11:41:34Z

Yeah, that captures my feelings, too. Are we fine with having a begin but no end, e.g., when disabling uncrustify for the whole file?

If SGML is allowed to do that then we are as well... ;-P.

miri64 · 2019-01-25T13:01:24Z

One word on portability: My current workplace setup (Ubuntu 16.04) isn't able to read the uncrustify-riot.cfg correctly

$ uncrustify -v
uncrustify 0.59
$ uncrustify --no-backup -c uncrustify-riot.cfg boards/hifive1/board.c 
uncrustify-riot.cfg:11 Unknown symbol 'indent_ternary_operator'
uncrustify-riot.cfg:87 unknown type 'PROTO_WRAP':uncrustify-riot.cfg:93 Unknown symbol 'disable_processing_cmt'
uncrustify-riot.cfg:94 Unknown symbol 'enable_processing_cmt'
Parsing: boards/hifive1/board.c as language C

But that is already the case in master so I don't mind, but Ubuntu 16.04 is still officially supported so our users might still use it so we should pay attention to not lose support for it. I also tested on a somewhat current Debian (docker image debian/stretch-slim with just uncrustify (0.64) and git installed manually). There only the indent_ternary_operator isn't recognized, so this PR is not affected.

miri64 · 2019-01-25T14:55:53Z

@jia200x what do you think about the new marker style?

jia200x · 2019-01-25T15:21:07Z

@jia200x what do you think about the new marker style?

Much better now :)

kaspar030 · 2019-01-25T15:34:56Z

One word on portability: My current workplace setup (Ubuntu 16.04) isn't able to read the uncrustify-riot.cfg correctly
$ uncrustify -v
uncrustify 0.59

Shit, 0.59 is seven years old. It was superceded by two new releases at the end of 2014. I hope we can just let user's know they'll need to upgrade... Isn't the latest Ubuntu LTS our "officially supported platform"?

kaspar030 · 2019-01-25T15:35:39Z

@miri64 @jia200x happy enough so I can squash? 😉

jia200x · 2019-01-25T15:36:12Z

I'm fine with it ;)

miri64 · 2019-01-25T15:41:16Z

Shit, 0.59 is seven years old. It was superceded by two new releases at the end of 2014. I hope we can just let user's know they'll need to upgrade... Isn't the latest Ubuntu LTS our "officially supported platform"?

In general, you shouldn't expect users to require their OS to update everytime the newest version pops up just because it is inconvenient to you. Even for stable or LTS. There are valid reasons not to (e.g. no administrative rights on their machine or because people choose a stable OS because they don't want to update that often). There should always be a grace period and IMHO that should be until the official EOL of that OS release. For 16.04 that is 2021.

However, in specific I don't mind this minor inconvenience for our code beautifier...

miri64 · 2019-01-25T16:19:41Z

@miri64 @jia200x happy enough so I can squash?

Yes, sorry.. This somehow got lost in my ramblings about portability 👼

miri64

Ran it on an Arch machine again. Works like a charm. As a general rule we should however add reasonings to these blocks as you did in #10867.

miri64 · 2019-01-25T17:29:44Z

boards/hifive1/board.c

@@ -96,7 +96,7 @@ void board_init_clock(void)
     /* Note: The range is limited to ~100MHz and depends on PLL settings */
    PRCI_set_hfrosctrim_for_f_cpu(CPU_DESIRED_FREQ, PRCI_FREQ_UNDERSHOOT);

-     /* disable uncrustify *INDENT-OFF* */
+     /* begin{code-style-ignore} */


Uncrustify moves this comment one space to the left. Why not fix it ;-)? (the code below probably needs to be moved as well though :-/)

Why not fix it ;-)?

unrelated! ;)

uncrustify: add indentation section comment

f56533b

kaspar030 added Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Area: tools Area: Supplementary tools labels Jan 25, 2019

kaspar030 requested a review from miri64 January 25, 2019 10:32

miri64 reviewed Jan 25, 2019

View reviewed changes

kaspar030 added 2 commits January 25, 2019 17:21

uncrustify: change en/disable markers

99d7996

boards/hifive1: adapt to RIOT uncrustify markers

674128e

kaspar030 force-pushed the uncrustify_change_disable_hint branch from b2df670 to 674128e Compare January 25, 2019 16:21

kaspar030 changed the title ~~uncrustify: change en/disable markers to "*auto-format-[on|off]*"~~ uncrustify: change en/disable markers Jan 25, 2019

kaspar030 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR labels Jan 25, 2019

miri64 approved these changes Jan 25, 2019

View reviewed changes

kaspar030 merged commit 6cd81db into RIOT-OS:master Jan 25, 2019

kaspar030 deleted the uncrustify_change_disable_hint branch January 25, 2019 21:40

danpetry modified the milestone: Release 2019.04 Mar 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

uncrustify: change en/disable markers #10865

uncrustify: change en/disable markers #10865

kaspar030 commented Jan 25, 2019

jia200x commented Jan 25, 2019

miri64 left a comment

kaspar030 commented Jan 25, 2019

miri64 commented Jan 25, 2019

miri64 commented Jan 25, 2019 •

edited

Loading

miri64 commented Jan 25, 2019

jia200x commented Jan 25, 2019

kaspar030 commented Jan 25, 2019

kaspar030 commented Jan 25, 2019

jia200x commented Jan 25, 2019

miri64 commented Jan 25, 2019

miri64 commented Jan 25, 2019

miri64 left a comment

miri64 Jan 25, 2019

kaspar030 Jan 25, 2019

uncrustify: change en/disable markers #10865

uncrustify: change en/disable markers #10865

Conversation

kaspar030 commented Jan 25, 2019

Contribution description

Testing procedure

Issues/PRs references

jia200x commented Jan 25, 2019

miri64 left a comment

Choose a reason for hiding this comment

kaspar030 commented Jan 25, 2019

miri64 commented Jan 25, 2019

miri64 commented Jan 25, 2019 • edited Loading

miri64 commented Jan 25, 2019

jia200x commented Jan 25, 2019

kaspar030 commented Jan 25, 2019

kaspar030 commented Jan 25, 2019

jia200x commented Jan 25, 2019

miri64 commented Jan 25, 2019

miri64 commented Jan 25, 2019

miri64 left a comment

Choose a reason for hiding this comment

miri64 Jan 25, 2019

Choose a reason for hiding this comment

kaspar030 Jan 25, 2019

Choose a reason for hiding this comment

miri64 commented Jan 25, 2019 •

edited

Loading