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.

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

AStyle: Fix astyle indentation #7007

Merged
merged 2 commits into from Jun 1, 2018

Conversation

Projects
None yet
6 participants
@0xc0170
Member

0xc0170 commented May 24, 2018

Description

This PR fixes the indentation we identified (thanks @kjbracey-arm @hasnainvirk) in Lorawan PR - lorawan codebase tested astyle to the point where we found that the code astyle was fixing was not correct (see the commit messages with examples how it was and with this config how is it). You will spot the differences.

This PR is vital for astyle applying to our bigger codebase.

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

0xc0170 added some commits May 24, 2018

AStyle: don't indent cases
This was a bug in our configuration. Earlier versions of our style did not
specify this option. We found out for some code in our codebase that it was
changed because of the setting.

An example:

```
case: {
    // code here
}

//would be changed to

case: {
    // code here
   }
```

The first one is correct and do not need to be indented (it's enough switch is
indented).
AStyle: fix indentation for longer lines
Long lines like this would have parameters misaligned but shorter lines would be OK.

```
 void LoRaMac::check_frame_size(uint16_t size)
 {
     uint8_t value = _lora_phy.get_max_payload(_mcps_indication.rx_datarate,
-                                              _params.is_repeater_supported);
+                    _params.is_repeater_supported);
+
+    _lora_phy.a(_mcps_indication.b,
+                b.is_repeater_supported);
```

With this option (applied to the code above), we get

```
@@ -319,6 +319,9 @@ void LoRaMac::check_frame_size(uint16_t size)
     uint8_t value = _lora_phy.get_max_payload(_mcps_indication.rx_datarate,
                                               _params.is_repeater_supported);

+    _lora_phy.a(_mcps_indication.b,
+                b.is_repeater_supported);
```

Both are aligned the same. However there is a limit of astyle - 120 columns for this.
We do in most cases lines from 80 to 120 as suggested in our code lines thus this should
be good.

@0xc0170 0xc0170 requested review from geky, hasnainvirk and ARMmbed/mbed-os-maintainers May 24, 2018

@hasnainvirk

Looks good.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 24, 2018

To bring bigger picture here, with these 2 options, using AStyle 3.1 as recommended this is the outcome to have astyle run for lorawan codebase: https://github.com/ARMmbed/mbed-os/compare/master...0xc0170:test?expand=1 (please review if all is good). It seems to me 👍

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 31, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented May 31, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@adbridge adbridge added ready for merge and removed needs: CI labels Jun 1, 2018

@adbridge adbridge merged commit c151811 into ARMmbed:master Jun 1, 2018

13 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/astyle Passed, 846 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10161 cycles (+486 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment