Add AStyle definition file and travis astyle check #6014
Conversation
0xc0170
changed the title
0xc0170
changed the title
.travis.yml
Outdated
| @@ -93,6 +93,24 @@ matrix: | |||
| # Report success since we have overridden default behaviour | |||
| - bash -c "$STATUS" success "Local $NAME testing has passed" | |||
|
|
|||
| - env: | |||
| - TEST="astyle" | |||
| before_install: | |||
There was a problem hiding this comment.
Unfortunately, Travis doesn't inherit from multiple before_installs.
If you want the status update + pipefail + caching (and probably just future compatibility), you'll need to copy them from the global before_install or move this to the install. Maybe a YAML reference will work?
There was a problem hiding this comment.
will check, and fix somehow.. I noticed that using default version it complains about options not available but their webpage lists them, so need to get update somehow , will fix
.travis.yml
Outdated
| install: | ||
| - sudo apt-get install astyle | ||
| script: | ||
| - export ARTISTIC_STYLE_OPTIONS=".astylerc" && astyle `find . | egrep "\.c|\.h|\.hpp|\.cpp"` > astyle.out |
There was a problem hiding this comment.
tiny nit: find can do this for you:
+ find -regex '.*\.\(h\|c\|hpp\|cpp\)'
- find . | egrep "\.c|\.h|\.hpp|\.cpp"
.travis.yml
Outdated
| install: | ||
| - sudo apt-get install astyle | ||
| script: | ||
| - export ARTISTIC_STYLE_OPTIONS=".astylerc" && $ find -regex '.*\.\(h\|c\|hpp\|cpp\)' -type f ! -path "./cmsis/*" ! -path "./features/mbedtls/*" ! -path "./features/FEATURE_LWIP/lwip/*" ! -path "./rtos/TARGET_CORTEX/rtx5/RTX/*" ! -path "./BUILD/*" ! -path "./tools/*" | xargs -n 100 -I {} bash -c "astyle --options=.astylerc \"{}\"" > astyle.out |
There was a problem hiding this comment.
@geky what magic can we use to exclude some directories ? I defined only some generic ones, but lot of targets contain their own drivers.
I tried to use --exclude, but always end up with error unmatched pattern from astyle 😢
Shall we define exclude paths in a file, or provide .astyleignore file that would contain those, we would read them and exclude from astyle.
|
Note: I've noticed AStyle in ubuntu that is used in travis is around v2.05, the reference manual on their official webpage is for 3.x :-) their latest version is published for unstable versions at the moment 😀 |
|
Looking at travis, the command seems to be working, I can see lot of diffs there for files that needs to be changed. Based on the output, here are proposed exclude dirs (along the ones that I already defined): |
.travis.yml
Outdated
| @@ -93,6 +95,20 @@ matrix: | |||
| # Report success since we have overridden default behaviour | |||
| - bash -c "$STATUS" success "Local $NAME testing has passed" | |||
|
|
|||
| - env: | |||
| - TEST="astyle" | |||
.travis.yml
Outdated
| install: | ||
| - sudo apt-get install astyle | ||
| script: | ||
| - export ARTISTIC_STYLE_OPTIONS=".astylerc" && find -regex '.*\.\(h\|c\|hpp\|cpp\)' -type f ! -path "./cmsis/*" ! -path "./features/mbedtls/*" ! -path "./features/FEATURE_LWIP/lwip/*" ! -path "./rtos/TARGET_CORTEX/rtx5/RTX/*" ! -path "./BUILD/*" ! -path "./tools/*" | xargs -n 100 -I {} bash -c "astyle --options=.astylerc \"{}\"" > astyle.out |
There was a problem hiding this comment.
ARTISTIC_STYLE_OPTIONS=".astylerc" could go in the env section
.astyleignore
Outdated
| features\FEATURE_BLE\targets\TARGET_Maxim\exactLE | ||
| features\FEATURE_BLE\targets\TARGET_NORDIC\TARGET_MCU_NRF51822\sdk\ | ||
| features\FEATURE_LWIP\lwip-interface\lwip | ||
| features\unsupported\ |
.astylerc
Outdated
| attach-classes | ||
|
|
||
| # Folders to be excluded | ||
| # exclude=cmsis |
|
I sent docs update to cover this addition and automatic check - ARMmbed/mbed-os-5-docs#400 Thanks @geky for help, the command works locally, and files changed with astyle are only those we care about (ignorefile is working) ! Once travis finishes, we review all changes and I can add a new commits here fixing the code until we make AStyle green |
|
The last travis run works with astyle, we can see all the styling issues now displayed t here. I'll add commits to fix those issues to make travis green |
0xc0170
force-pushed
the
dev_astyle
branch
2 times, most recently
from
8b1e16c
to
bc0091b
Compare
| touched = true; | ||
| TEST_ASSERT_EQUAL(a0 | a1, 0x3); | ||
| } | ||
|
|
||
| void func1(int a0) { | ||
| void func1(int a0) | ||
| { |
There was a problem hiding this comment.
I was curious about this so I tried astyle with both brace styles:
with style=kr: 1841070 line changes
with style=attach: 1819522 line changes
It would actually take fewer code changes to change the existing KR braces than to standardize on KR. We should reconsider our stance on brace placement.
I've never understood why our brace style is inconsistent between functions and every other syntactic construct.
There was a problem hiding this comment.
It would actually take fewer code changes to change the existing KR braces than to standardize on KR. We should reconsider our stance on brace placement.
I have testing online compiler - we need to match what we have here and what online compiler does. It seems it defines a rule to actually attach { in the header files, will run few more tests and get its options that it defines (only for C++ functions), AStyle has this option (attach-inlines) that seems to be active in the online compiler
The line 59 is correct, what we need to align is C++ inlines. From experience, not always it was clear when to attach { for users thus our initial coding style set it always new line for functions (no matter if C++ or C). This however as we can see it is not aligned with online compiler, will check
There was a problem hiding this comment.
I updated this PR, attached C++ as the online compiler does.
There was a problem hiding this comment.
This contradict the guidelines and guidelines are made to be followed; the purpose of this PR is to enforce them. If the online compiler is wrong; fix the online compiler. Not the other way around.
There was a problem hiding this comment.
KR style braces are still the minority, although only slightly:
with style=kr: 1819759 line changes
with style=attach: 1819522 line changes
I'm suggesting we change the guidelines to match the style of code users are contributing. I had no idea C++ inlines are a separate style, and if we don't know what our rules are on braces how are our users supposed to?
I mean what is this:
namespace hi {
class Thing {
int dothing() {
if (thing) {
blahblahblah;
} else {
blahblahblah;
}
}
};
int dothing()
{
while (thing) {
blahblahblah;
}
}
}What's the reason for not making squiggly brackets consistent?
|
Nanostack and the related "COMMON_PAL" code are subtrees from master external repos - I think it would be less disruptive to format them over there then merge, initially. Incoming subtree merges can certainly be checked for formatting. |
I'll remove nanostack and common pal here |
|
To be a little more precise - these are from out-of-tree:
These are mbed OS specific components, mastered in-tree and can be reformatted:
|
|
Comparing to the online IDE formatter, what we do in the codebase and define even in our code style: What we had earlier defined was I rebased this, to clean the history a bit and reapply the coding style. |
|
Also damn you grammar! |
|
This is good (showcase how it works when it fails)! I'll rebase to fix the code style issue. |
|
@0xc0170 Since this touches a whole bunch of files, I'm thinking this will need to either
It looks like it will be ready after another rebase. |
I fear that this might be the case. However if the style is approved, still this can land partially (astyle check for new code only - so we start slowly adapting the new code - should be fairly simple to update and send via a new PR), and we update the older code for 5.9. Open for suggestions. I am not rebasing again, so will wait until we finalize the plan |
|
@0xc0170 I think the style checks as they are now are fine, and think everyone else is now onboard with them (at least, no one as raised any other concerns with them). I think we could take the following approach:
(In case it wasn't clear, each of those above steps would be an individual PR. #3 would involve multiple PRs.) |
|
+1 for the astyle travis check as a warning until 5.9! |
|
@0xc0170 +1 for the idea and the energy put in this. To make this less disruptive I think that we need to:
|
|
This serves as an example for astyle to see all the code that is affected. I am preparing smaller one that is only adding travis and astyle config. It will check only files that has changed, once I have it running will reference here and close this one. |
This should do it #6590 . Once this one is in, I'll start with the code base one by one I am closing this for now |
@geky Your latest PR was going the good direction. I defined astyle rc based on the style we have in this codebase and added it to Travis
This is work in progress, as we need to actually define what we should be ignoring (I defined few I could think of, however there are external code in our codebase like SDK, network stacks, etc). Therefore this needs more work to be done (I could not find actually that --exclude would support wildcards?? for astyle).
I run locally on drivers only, lot of changes done in files. Thus travis should fail and point to all files that need changes.