[riot-notifications] [RIOT-OS/RIOT] uncrustify: add auto uncrustify with blacklist (#8519)

José Alamos notifications at github.com
Fri Jan 25 13:56:39 CET 2019


Hi @kaspar030 

I completely agree with your comments and agree we should adapt our coding convention a bit to make it easier for us. I also think the corner cases don't matter that much and I prefer to add them to the coding convention instead of  /* INDENT-ON */ and /* INDENT-OFF */ everywhere.

So up to this point, I agree we should improve our Uncrustify integration and it should be tool ran by RIOT developers.


My whole point of this PR is there are 2 different topics:
1. Code beautifying
2. Code-style checking

(1) Is solved by Uncrustify. (2) Is solved by a code-style checker like Vera++

IMO from the CI we should check the style convention and report **why** there's an error somewhere and **where** is the error. The intention of this PR was to add a CI helper, that's why I closed it.

This IMO can't be done properly with Uncrustify.

Please check [this gist](https://gist.github.com/jia200x/f17c6c9316f4f35bf151fab0b112fd7a) with Uncrustify and Vera++ results fro style checking.
I took as an example the xtimer_core.c file (original.c) and ran uncrustify with our current RIOT config (uncrustify.c).

I ran Vera++ in the original file (output at vera_original.txt) and in the uncrustified version (vera_uncrustify.txt).
I also got diff between original.c and uncrustify.c (diff.txt).

Some conclusions:
1. Uncrustify diff shows [some unrelated changes like this](https://gist.github.com/jia200x/f17c6c9316f4f35bf151fab0b112fd7a#file-diff-txt-L15). I wouldn't put a `/* INDENT-OFF */` there because I think it looks more beautiful, but this line is not violating our standard.

2. It's not possible to know **why** there was an issue. Check e.g this snippet:
```
31c531
<         while (reference < _xtimer_lltimer_now()) {}
---
> while( reference < _xtimer_lltimer_now()){}
```

Just by looking the output, what's the error?

Vera++'s original.c outputs:
```
/tmp/original.c:531: keyword 'while' not followed by a single space
```

It's possible to know **where** is the issue by diff output, but if it's unsure if it's an issue then there would be tons of false positives.
And also, isn't Vera++ output exactly what we would like to have in the CI?

3. After running Uncrustify, Vera++ output improves. So it can still increase the chances a PR passes CI style checks.


>- from this PR, remove the "uncrustify-force" config, just use "uncrustify-riot.conf" from the root in addition to the blacklist file, add a whitelist. Start small (e.g., just with core/).
>- get consensus on reduced maintanance / review time over "I prefer this or that style", make "uncrustify-  riot.cfg" authorative
>- add a message to the coding convention for that
> - add a wiki page explaining why and how uncrustify

I agree.

I also propose:
- Adapt this PR to be a helper for the user, not for the CI.
- As discussed previously, enable Vera++ in Travis.


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/8519#issuecomment-457564649
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190125/f13f109e/attachment-0001.html>


More information about the notifications mailing list