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

José Alamos notifications at github.com
Fri Jan 25 15:32:55 CET 2019


> Well, I mean, CI runs uncrustify in "--check" mode. The user might then want to run uncrustify actually applying the changes.

NB the only thing we can report is wether uncrustify was ran or not (diff != empty). We cannot report what were the errors. 

> To me this defeats the purpose of an automatic code formatting tool, if the developer does not run it.

That's the point, Vera++ is not a code formatting/beautifier tool. It's just a rule-based code style checker.
[It's straightforward to write RIOT convention with Vera++ rules](https://bitbucket.org/verateam/vera/wiki/Rules) and can detect if a rule is violated. Uncrustify parses the C code and rebuilds it based on rules. All information about broken rules is lost.

> Please let us converge to a point where we do not have to deal with code formatting anymore.

If the CI runs coding style checks, we don't have to care about coding style anymore.

E.g these 2 cases:
- Uncrustify running on CI: We can either report:
```

217,222c217,222
<     * Backoff condition above ensures that 'target - XTIMER_OVERHEAD` is later
<     * than 'now', also for values when now will overflow and the value of target
<     * is smaller then now.
<     * If `target < XTIMER_OVERHEAD` the new target will be at the end of this
<     * 32bit period, as `target - XTIMER_OVERHEAD` is a big number instead of a
<     * small at the beginning of the next period. */
---
>      * Backoff condition above ensures that 'target - XTIMER_OVERHEAD` is later
>      * than 'now', also for values when now will overflow and the value of target
>      * is smaller then now.
>      * If `target < XTIMER_OVERHEAD` the new target will be at the end of this
>      * 32bit period, as `target - XTIMER_OVERHEAD` is a big number instead of a
>      * small at the beginning of the next period. */
269c269
<                || (((*list_head)->long_target == timer->long_target) && ((*list_head)->target <= timer->target)))) {
---
>            || (((*list_head)->long_target == timer->long_target) && ((*list_head)->target <= timer->target)))) {
531c531
<         while (reference < _xtimer_lltimer_now()) {}
---
> while( reference < _xtimer_lltimer_now()){}
```

or something like 

```
uncrustify --check failed! Please run Uncrustify
```

- Vera++ running on CI. It would report:

```
/tmp/original.c:269: line is longer than 100 characters
/tmp/original.c:404: line is longer than 100 characters
/tmp/original.c:502: line is longer than 100 characters
/tmp/original.c:531: keyword 'while' not followed by a single space
```

In both cases, **the user would need to run Uncrustify anyway**.

But we have a more detailed report with Vera++, it won't give false positives if we change its rules and the most important thing IMO: we can already run Vera++ _now_  without adding `/* INDENT-OFF */` tags and would check the whole repo as it is.
This doesn't mean we shouldn't gradually add `/* INDENT-OFF */` tags, but at least we can already enforce code style for all RIOT files ASAP with Vera++.

In fact, that's the way how I've been checking coding style on PRs.

-- 
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-457590852
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190125/43ee8562/attachment.html>


More information about the notifications mailing list