Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

features: fix coding style #8591

Merged
merged 18 commits into from
Nov 9, 2018
Merged

features: fix coding style #8591

merged 18 commits into from
Nov 9, 2018

Conversation

0xc0170
Copy link
Contributor

@0xc0170 0xc0170 commented Oct 30, 2018

Description

AStyle run on all features that we have (ignored folders are not being checked). This needs further work (we might not have ignore folders up to date, or some features might need update upstream). Let's review what can be updated and how.

I'll add reviewers with questions about the features and its style (some might need some edits).

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

@0xc0170 0xc0170 changed the title Fix coding style features features: fix coding style Oct 30, 2018
@0xc0170
Copy link
Contributor Author

0xc0170 commented Oct 30, 2018

travis-ci/astyle — Passed, 66 files (-480 files)

Need to find these 66 files 👀

@0xc0170
Copy link
Contributor Author

0xc0170 commented Oct 30, 2018

Initial feedback for files changed here:

@ARMmbed/mbed-os-storage
@ARMmbed/mbed-os-ipcore
@ARMmbed/mbed-os-pan
@ARMmbed/mbed-os-wan

Please review your features here and let me know what can be run through astyle (I used .astyleignore paths to ignore), so if files here are changed, means I need to rerun it with updated ignored folders.

pan-
pan- requested changes Oct 30, 2018
View reviewed changes
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already too many comments and I'm not half way in for BLE. Astyle is very dumb for some constructs.

features/FEATURE_BLE/ble/ArrayView.h Outdated
@@ -102,8 +103,8 @@ struct ArrayView {
* @post a call to size() will return Size, and data() will return
* a pointer to elements.
*/
ArrayView(T (&elements)[Size]):
_array(elements) { }
ArrayView(T(&elements)[Size]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Astyle nonsense:

Suggested change
ArrayView(T(&elements)[Size]):
ArrayView(T (&elements)[Size]):

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is known.

We need to turn off astyle for any template magic, similar in Span class and others. I'll fix that . Thanks for the pointers !

Main question - is there any BLE code that should be excluded apart from this errors?

features/FEATURE_BLE/ble/ArrayView.h Outdated
@@ -203,7 +204,7 @@ struct ArrayView<T, ARRAY_VIEW_DYNAMIC_SIZE> {
* a pointer to elements.
*/
template<size_t Size>
ArrayView(T (&elements)[Size]):
ArrayView(T(&elements)[Size]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Astyle nonsense:

Suggested change
ArrayView(T(&elements)[Size]):
ArrayView(T (&elements)[Size]):

features/FEATURE_BLE/ble/ArrayView.h Outdated
@@ -333,7 +334,7 @@ bool operator!=(const ArrayView<T, LhsSize>& lhs, const ArrayView<T, LhsSize>& r
* created 'inline'.
*/
template<typename T, size_t Size>
ArrayView<T, Size> make_ArrayView(T (&elements)[Size])
ArrayView<T, Size> make_ArrayView(T(&elements)[Size])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Astyle nonsense:

Suggested change
ArrayView<T, Size> make_ArrayView(T(&elements)[Size])
ArrayView<T, Size> make_ArrayView(T (&elements)[Size])

features/FEATURE_BLE/ble/ArrayView.h Outdated
@@ -389,7 +390,7 @@ ArrayView<T> make_ArrayView(T* array_ptr, size_t array_size)
* created 'inline'.
*/
template<typename T, size_t Size>
ArrayView<const T, Size> make_const_ArrayView(T (&elements)[Size])
ArrayView<const T, Size> make_const_ArrayView(T(&elements)[Size])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Astyle nonsense:

Suggested change
ArrayView<const T, Size> make_const_ArrayView(T(&elements)[Size])
ArrayView<const T, Size> make_const_ArrayView(T (&elements)[Size])

features/FEATURE_BLE/ble/BLE.h Outdated
@@ -471,7 +473,8 @@ class BLE
MBED_DEPRECATED("Use ble.gap().getAddress(...)")
ble_error_t getAddress(
BLEProtocol::AddressType_t *typeP, BLEProtocol::AddressBytes_t address
) {
)
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{ for functions is not attached

features/FEATURE_BLE/ble/GattCharacteristic.h Outdated
descriptors,
numDescriptors
) {
uuid,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

features/FEATURE_BLE/ble/GattCharacteristic.h Outdated
descriptors,
numDescriptors
) {
uuid,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

features/FEATURE_BLE/ble/GattCharacteristic.h Outdated
descriptors,
numDescriptors
) {
uuid,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

features/FEATURE_BLE/ble/GattCharacteristic.h Outdated
numDescriptors,
false
) {
uuid,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

features/FEATURE_BLE/ble/GattCharacteristic.h Outdated
descriptors,
numDescriptors
) {
uuid,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above.

pan-
pan- previously requested changes Oct 30, 2018
View reviewed changes
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already too many comments and I'm not half way in for BLE. Astyle is very dumb for some constructs.

AnttiKauppila
AnttiKauppila approved these changes Oct 31, 2018
View reviewed changes
Copy link

@AnttiKauppila AnttiKauppila left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoRaWAN and Cellular are fine

@0xc0170
Copy link
Contributor Author

0xc0170 commented Oct 31, 2018

@AnttiKauppila Thank you!

@pan- I left one comment above, and will review all instances of BLE issues (If it makes easier, I can send separate PR). Let me know what would be the best approach for BLE code.

@paul-szczepanek-arm
Copy link
Member

paul-szczepanek-arm commented Oct 31, 2018 •
edited
Loading

Instead of making exceptions and fixing errors one by one maybe it would be worth it to come up with a set of astyle rules that you agree on? Most of the changes here could be fixed with a config change. Not all though but AStyle is open source and reasonably easy to modify. Vincent's function call and declaration style will probably require code modification to be applied and/or retained.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Oct 31, 2018

Instead of making exceptions and fixing errors one by one maybe it would be worth it to come up with a set of astyle rules that you agree on?

Already done via the config file we have - if you spot we are missing something, we can fix it.
These issues that @pan- found are astyle shortcomings. See their docs, this is from their docs

Artistic Style cannot always determine the usage of symbols with more than one meaning. For example an asterisk (*) can be multiplication, a pointer, or a pointer dereference. The "&" and "&&" symbols are a similar problem.

If a symbol is being padded incorrectly, padding it manually may fix the problem. If it is still being padded incorrectly, then disabling the formatting may be necessary. To avoid having to use the "disable block" tags above, a single line disable is available.

Most of the changes here could be fixed with a config change. Not all though but AStyle is open source and reasonably easy to modify.

I am not certain we can fix easily the above (*,&,&&).

From the experience, some code use different formatting than what we have in the config and astyle formats it wrongly anyway - manual update resolves it. I'll review all the review comments soon

@paul-szczepanek-arm
Copy link
Member

paul-szczepanek-arm commented Oct 31, 2018 •
edited
Loading

Some of these seem to be caused by unpad-paren not being clever enough. Sadly there is no unpad-paren-in

@paul-szczepanek-arm
Copy link
Member

The strength of astyle is that's easy to modify because it's so simple but the drawback of astyle is that it's so simple and doesn't understand the code it's styling.

There are some alternatives that are more langauge aware. When I got frustrated with astyle I switched to https://github.com/uncrustify/uncrustify. Using http://universalindent.sourceforge.net/ you could recreate the existing astyle config very quickly and then improve on it as uncrustify has more fine grained options and doesn't have the same bugs astyle has.

Apologies if this was already considered in the past.

@0xc0170
Copy link
Contributor Author

0xc0170 commented Oct 31, 2018

Instead of focusing on the tool itself (thanks for the tip - haven't used it previously, we shall review uncrustify and clang format for future use. We need to work with the tools we have now), lets focus on the code and tools we have been using.

Most of the review comments are related to the hard character limit - in this case 80 (lines are split). We haven't enforced this limit (our coding style suggested 120 but not enforced) thus would suggest reformat the code we identified - should be quick to do (run astyle and review as done here) and would align with the rest of the codebase (look at lorawan, had similar style issues). This should fix most of the issues that we identified here (multiline typedef, parameters to the object ctor and similar). If you got any other suggestions, lets discuss it in the new PR. I suggest removing BLE from here, create a new PR and reformat the code - done by @ARMmbed/mbed-os-pan team as they have better understanding of the codebase.

I'll stash my changes for BLE just in case but will push an update to exclude BLE here in this pull request. Lets focus on the rest of features here

@0xc0170
Copy link
Contributor Author

0xc0170 commented Oct 31, 2018

Done , BLE feature excluded (added to the astyleignore file), will be fixed via separate PR.

@0xc0170 0xc0170 dismissed pan-’s stale review October 31, 2018 10:33

BLE in separate PR

@0xc0170
Copy link
Contributor Author

0xc0170 commented Oct 31, 2018

Added also cleanup for the rest of the codebase (to check that ignore folders are also up to date, added new ignore folders) and this is now ready for review!

travis-ci/astyle — Passed, 29 files (-516 files)

Most probably master and its ignore not up to date. I run astyle on entire codebase, all files should be resolved with this pull request (besides what we merge in the meantime)

@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 5, 2018

@ARMmbed/mbed-os-storage Please review features/storage changes
@ARMmbed/mbed-os-ipcore Please review nanostack/lwip/netsocket

The rest of changes are minimal update should be sufficient to be reviewed by @ARMmbed/mbed-os-maintainers

@0xc0170 0xc0170 requested review from a team November 5, 2018 09:50
geky
geky approved these changes Nov 6, 2018
View reviewed changes
Copy link
Contributor

@geky geky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't look too closely, but storage looks reasonable to me (not much advanced c++ there)

@geky geky requested a review from a team November 6, 2018 14:55
davidsaada
davidsaada approved these changes Nov 7, 2018
View reviewed changes
Copy link
Contributor

@davidsaada davidsaada left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Storage changes look good. Thanks for this change.

@0xc0170 0xc0170 mentioned this pull request Nov 8, 2018
Merged
@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 8, 2018

/morph build

@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 8, 2018

I rebased to resolve one bad rebase, this is going alone (no rollup)

@0xc0170 0xc0170 removed the rollup PR label Nov 8, 2018
@0xc0170 0xc0170 mentioned this pull request Nov 8, 2018
Merged
@mbed-ci
Copy link

mbed-ci commented Nov 8, 2018

Build : FAILURE

Build number : 3579
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8591/

@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 8, 2018

CI issues in the last failures, we are investigating

cc @OPpuolitaival

@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 8, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Nov 8, 2018

Build : SUCCESS

Build number : 3584
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8591/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

@mbed-ci
Copy link

mbed-ci commented Nov 8, 2018

Exporter Build : SUCCESS

Build number : 3194
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8591/

@mbed-ci
Copy link

mbed-ci commented Nov 8, 2018

Test : FAILURE

Build number : 3358
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8591/3358

@cmonr
Copy link
Contributor

cmonr commented Nov 8, 2018

Test timeout appears spurrious. Will retest when able.

@cmonr
Copy link
Contributor

cmonr commented Nov 9, 2018

/morph test

@mbed-ci
Copy link

mbed-ci commented Nov 9, 2018

Test : SUCCESS

Build number : 3362
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8591/3362

@0xc0170
Copy link
Contributor Author

0xc0170 commented Nov 9, 2018

All green 🍏 💚 ready for integration!

@cmonr cmonr merged commit 9d95d46 into ARMmbed:master Nov 9, 2018
@0xc0170 0xc0170 deleted the fix_coding_style_features branch November 9, 2018 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VeijoPesonen VeijoPesonen VeijoPesonen approved these changes

@davidsaada davidsaada davidsaada approved these changes

@AnttiKauppila AnttiKauppila AnttiKauppila approved these changes

@SeppoTakalo SeppoTakalo SeppoTakalo approved these changes

@geky geky geky approved these changes

@pan- pan- pan- left review comments

Assignees
No one assigned
Labels
release-version: 5.11.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.