tests: astyle fix #7605
Merged
tests: astyle fix #7605
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
0xc0170
force-pushed
the
fix_astyle_tests
branch
from
1c2d63b
to
8123516
Compare
|
Rebased, fixed one issue with flash hal test, compilation locally OK |
mprse
reviewed
Jul 30, 2018
TESTS/mbed_hal/crc/main.cpp
Outdated
| /* 32 */{ {POLY_32BIT_ANSI , 32, 0x00000000, 0xABABABAB, false, false}, 0x220A22D4 }, | ||
| /* 33 */{ {POLY_32BIT_ANSI , 32, 0x00000000, 0x00000000, true , false}, 0x11B4BFB4 }, | ||
| /* 34 */{ {POLY_32BIT_ANSI , 32, 0x00000000, 0x00000000, false, true }, 0xFE918591 }, | ||
| /* 00 */{ {POLY_7BIT_SD, 7, 0x00000000, 0x00000000, false, false}, 0xEA }, |
There was a problem hiding this comment.
Format of test cases array has been changed and and readability has been decreased.
There was a problem hiding this comment.
We can see that astyle removed spaces after macros (true/false including) but not after the numbers). I would completely remove those spaces there and just align it vertically as it is (one empty line between each group).
/* 42 */{ {POLY_16BIT_MAXIM, 16, 0x00000000, 0x00000000, false, false}, 0xFEE8 },
to
/* 42 */{{POLY_16BIT_MAXIM, 16, 0x00000000, 0x00000000, false, false}, 0xFEE8},
We can turn off style checking for these cases.
There was a problem hiding this comment.
Fixed now, I removed the spaces, should look now good as it is.
All tests should comply to our coding standard now
0xc0170
force-pushed
the
fix_astyle_tests
branch
from
8123516
to
90ff35b
Compare
|
Rebased, please review |
|
|
|
IMAO readability of the original version is still better. Maybe it would be good to make some exception for such tables, since readability is here the most important. In this case it is not so bad, but if length of input values was more diverse, it would be much harder to read this. Original: Fixed: |
jamesbeyond
reviewed
Aug 3, 2018
| { | ||
| return t | a0 | a1 | a2 | a3; | ||
| } | ||
| T member_func5(T a0, T a1, T a2, T a3, T a4) |
There was a problem hiding this comment.
These not look as neat as the old version, but if this is the coding style rules, I have no objection to it.
There was a problem hiding this comment.
Yes, that is how it is
| return t | a0 | a1 | a2 | a3; | ||
| } | ||
| T member_func5(T a0, T a1, T a2, T a3, T a4) | ||
| { |
TESTS/mbed_hal/crc/main.cpp
Outdated
| /* 52 */{{POLY_32BIT_POSIX, 32, 0x00000000, 0xFFFFFFFF, false, false}, 0x765E7680}, | ||
| /* 53 */{{POLY_32BIT_POSIX, 32, 0x00000000, 0xABABABAB, false, false}, 0x220A22D4}, | ||
| /* 54 */{{POLY_32BIT_POSIX, 32, 0x00000000, 0x00000000, true, false}, 0x11B4BFB4}, | ||
| /* 55 */{{POLY_32BIT_POSIX, 32, 0x00000000, 0x00000000, false, true }, 0xFE918591}, |
There was a problem hiding this comment.
astyle break the alignment a bit.
fkjagodzinski
approved these changes
Aug 3, 2018
There was a problem hiding this comment.
Although I don't have a strong preference for formatting, I do understand @mprse here. We could enclose such aligned tables in a section with different astyle format. I'm not sure if this tool provides such option though.
TESTS/mbed_hal/crc/main.cpp
Outdated
| /* 03 */{{POLY_7BIT_SD, 7, 0x00000000, 0x0000007F, false, false}, 0x95}, | ||
| /* 04 */{{POLY_7BIT_SD, 7, 0x00000000, 0x0000002B, false, false}, 0xC1}, | ||
| /* 05 */{{POLY_7BIT_SD, 7, 0x00000000, 0x00000000, true, false}, 0xA4}, | ||
| /* 06 */{{POLY_7BIT_SD, 7, 0x00000000, 0x00000000, false, true }, 0x57}, |
There was a problem hiding this comment.
There are still redundant spaces here.
Leave as it is for readability
0xc0170
force-pushed
the
fix_astyle_tests
branch
from
90ff35b
to
5f39232
Compare
I find readability for both the same. I reverted that manual change, and left the table as it was. It should be OK now |
mprse
approved these changes
Aug 3, 2018
mprse
approved these changes
Aug 3, 2018
|
/morph build |
Build : SUCCESSBuild number : 2730 Triggering tests/morph test |
Exporter Build : SUCCESSBuild number : 2366 |
Test : SUCCESSBuild number : 2460 |
|
/morph uvisor-test |
|
@ARMmbed/mbed-os-maintainers Needs one of your review and should be good to go |
cmonr
approved these changes
Aug 6, 2018
|
Interesting that astyle has a method of ignoring code blocks. |
|
This sits on top of changes only for 5.10, thus moving to 5.10.1 |
cmonr
added
release-version: 5.10.2
release-version: 5.10.0-rc1
and removed
release-version: 5.10.2
release-version: 5.10.1
labels
|
It turns out that because this was merged before rc1 was made, it's already in! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All tests should now comply to our coding standard
Description
Run astyle for all test files (TESTS/*)
I've noticed that inlined assembly might be broken, will test locally and fix (astyle should ignore inline assembly as such change as indentation there can lead to a broken code).
Pull request type