AStyle : drivers/hal/platform folders update #7008

0xc0170 · 2018-05-24T16:41:11Z

Description

This depends on #7007 (first 2 commits here are from that PR, will be rebased) . Applied to drivers/hal/platform folders. Up for review to see how astyle formats the code .

Not critical for 5.9, can come right after the code freeze so wont cause much rebasing (styling changes, should not change any behavior!)

This will need to be rebase as there are couple of PR changing this folders, will do final rebase once approved, thus please look at changes to check the style - the very important

Pull request type

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

0xc0170 · 2018-05-25T10:59:51Z

I fixed the rebase error, now compiles as it should ! :-)

cmonr · 2018-05-31T16:08:40Z

Gonna need another rebase.

0xc0170 · 2018-06-04T13:14:08Z

Rebased and updated ( I run astyle again to check if 3 folders that I am touching here are up to date with astyle).

0xc0170 · 2018-06-05T13:50:56Z

Note, I'll rebase this as final rebase once I get approvals (otherwise I daily rebase and those conflicts are not that simple sometimes as I have to run astyle for all conflicts over again).

cmonr

A couple of comments and questions, one of which I really care about.

cmonr · 2018-06-13T18:38:28Z

drivers/Ethernet.cpp

-        case HalfDuplex100 : speed = 1;  duplex = 0; break;
-        case FullDuplex100 : speed = 1;  duplex = 1; break;
+    switch (mode) {
+        case AutoNegotiate :


Do case statements need braces?

Also, should the space between case <name> : not be there?

Do case statements need braces?

No.

Also, should the space between case : not be there?

Yes, seems like astyle bug! (it uses the same formatting as previous one).

cmonr · 2018-06-13T18:40:30Z

drivers/InterruptManager.cpp

            delete _chains[i];
+        }


I'm just going to assume this corresponds to the brace on line 75.

Part of NULL check, line 79

cmonr · 2018-06-13T18:42:01Z

drivers/SPI.cpp

+    _mode(0),
+    _hz(1000000),
+    _write_fill(SPI_FILL_CHAR)
+{


This brace set feels like it should have an intentation.

function body, no indentation needed

cmonr · 2018-06-13T18:43:44Z

hal/mbed_itm_api.c

    }
 }

 uint32_t mbed_itm_send(uint32_t port, uint32_t data)
 {
    /* Check if ITM and port is enabled */
    if (((ITM->TCR & ITM_TCR_ITMENA_Msk) != 0UL) &&      /* ITM enabled */
-        ((ITM->TER & (1UL << port)     ) != 0UL)   )     /* ITM Port enabled */
-    {
+            ((ITM->TER & (1UL << port)) != 0UL)) {           /* ITM Port enabled */


This style change actually upsets me.

Imo, its much easier to read multiline conditional statements when the left parenthesis are aligned based on their parenthesis depth level. This change throws all of that out the window.

That is the default behavior we use (no config in the astylerc, neither in our code style guide for this). Parameters are aligned if on multilines but conditions are indented.

The config to check for this is --min-conditional-indent=# / -m#.

0xc0170 · 2018-06-15T12:46:54Z

Rebased to resolve conflicts .

cmonr · 2018-06-19T01:12:48Z

@ARMmbed/mbed-os-maintainers Would anyone else like to review?
What about @ARMmbed/mbed-os-core and/or @ARMmbed/mbed-os-hal?

ithinuel

There is some inconsistencies with block comments for functions and structure members.
For example the comments lines 84-85 in Driver_Storage.h should be aligned with the comment line 83 as they are part of the same member documentation.

And this function doc should probably aligned like the others.

Also I know it is part of K&R but as we already have 2 exceptions to the rules, one for the function's opening brace's new line would be good to me.
The line return doesn't help much readability while artificially increasing LOC count.
It should be consistent with other blocks indentation (for/while/if/else).

0xc0170 · 2018-06-20T10:10:55Z

@ithinuel Driver_Storage changes seems to be a bug in AStyle, astyle do not format doxygen. See the bug report: https://sourceforge.net/p/astyle/bugs/409/ - Won't fix. Here are my findings:

If I change the style for the comment, this stays as it is (line 2 is intentionally misalinged to check if it aligns it or leaves it) :

    uint32_t                      program_unit;         /**<  Minimum programming size in bytes.
The offset of the start of the program-range should also be aligned with this value.
                                                              Applicable only if the 'programmable' attribute is set for a block.
                                                              @note: setting program_unit to 0 has the effect of disabling the size and alignment
                                                              restrictions (setting it to 1 also has the same effect).  */

From the astyle docs: no doxygen mentioned in the documentation (no option there available). There are 3 tickets in the issue tracker that are related to doxygen, none resolved. I assume AStyle treats ///< as a code comment (//) and that one would align. See how it affects if I do (//)

    uint32_t                      program_unit;         /**<  Minimum programming size in bytes.
The offset of the start of the program-range should also be aligned with this value.
                                                              Applicable only if the 'programmable' attribute is set for a block.
                                                              @note: setting program_unit to 0 has the effect of disabling the size and alignment
                                                              restrictions (setting it to 1 also has the same effect).  */
    uint32_t                      optimal_program_unit; // Optimal programming page-size in bytes. Some storage controllers
    //   have internal buffers into which to receive data. Writing in chunks of
    //   'optimal_program_unit' would achieve maximum programming speed.
    //   Applicable only if the 'programmable' attribute is set for the underlying block(s).

It touches CRC header file just because it uses 5 spaces so aligns it to 4. But does not touch comments (no doxygen support). Looks fine. Formatting doxygen is out of scope for AStyle.

To resolve the Storage problem, either ignore that file (its deprecated anyway, I should probably not do any style changes. I'll revert them) or use other doxygen formatting (as shown above /**< */ works). I checked the code base, this is the only instance of multiline comments that use this syntax.

Also I know it is part of K&R but as we already have 2 exceptions to the rules, one for the function's opening brace's new line would be good to me.
The line return doesn't help much readability while artificially increasing LOC count.
It should be consistent with other blocks indentation (for/while/if/else).

I dont follow.

0xc0170 · 2018-06-20T10:29:04Z

I rebased. hal/storage_abstraction was removed from formatting, won't change those files. Thanks for noticing this @ithinuel

0xc0170 · 2018-06-27T14:11:18Z

Rebased again ! up to date

Deprecated API thus no changes there

0xc0170 · 2018-06-29T10:48:27Z

This reduces currently from 913 files needs code style update to 790 .

0xc0170 · 2018-06-29T10:48:34Z

/morph build

mbed-ci · 2018-06-29T11:57:14Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-06-29T13:21:24Z

Test : SUCCESS

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

mbed-ci · 2018-06-29T13:26:14Z

Exporter Build : SUCCESS

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

cmonr · 2018-06-30T01:40:46Z

@ithinuel Are you all good with the changes?

0xc0170 · 2018-07-02T14:24:34Z

@cmonr I spoke with @ithinuel , all good here

0xc0170 requested review from a team May 24, 2018 16:41

0xc0170 added needs: review needs: work and removed needs: review labels May 24, 2018

0xc0170 force-pushed the fix_drivers branch from 5d4a87e to f96480a Compare May 25, 2018 10:02

0xc0170 added needs: review and removed needs: work labels May 25, 2018

0xc0170 changed the title ~~Astyle : drivers/hal/platform folders update~~ AStyle : drivers/hal/platform folders update May 25, 2018

0xc0170 added needs: work and removed needs: review labels Jun 4, 2018

0xc0170 force-pushed the fix_drivers branch from f96480a to abb16e4 Compare June 4, 2018 13:13

0xc0170 added needs: review and removed needs: work labels Jun 4, 2018

cmonr added needs: preceding PR and removed needs: preceding PR labels Jun 13, 2018

cmonr reviewed Jun 13, 2018

View reviewed changes

0xc0170 force-pushed the fix_drivers branch from abb16e4 to c333eb5 Compare June 15, 2018 12:46

0xc0170 requested a review from a team June 19, 2018 13:47

deepikabhavnani previously approved these changes Jun 19, 2018

View reviewed changes

ithinuel suggested changes Jun 20, 2018

View reviewed changes

0xc0170 dismissed deepikabhavnani’s stale review via 74af4e8 June 20, 2018 10:28

0xc0170 force-pushed the fix_drivers branch from c333eb5 to 74af4e8 Compare June 20, 2018 10:28

0xc0170 force-pushed the fix_drivers branch from 74af4e8 to 3694237 Compare June 27, 2018 14:09

0xc0170 added 4 commits June 29, 2018 10:36

drivers: astyle update

700e6df

AStyle: ignore hal storage abstraction

d10e821

Deprecated API thus no changes there

hal: astyle update

483427a

platform: astyle update

ffcb6ec

0xc0170 force-pushed the fix_drivers branch from 3694237 to ffcb6ec Compare June 29, 2018 09:39

cmonr approved these changes Jun 30, 2018

View reviewed changes

0xc0170 added ready for merge release-version: 5.9.3 and removed needs: review labels Jul 2, 2018

0xc0170 merged commit 4005887 into ARMmbed:master Jul 2, 2018

0xc0170 removed the ready for merge label Jul 2, 2018

AStyle : drivers/hal/platform folders update #7008

AStyle : drivers/hal/platform folders update #7008

Uh oh!

Conversation

0xc0170 commented May 24, 2018

Description

Pull request type

Uh oh!

0xc0170 commented May 25, 2018

Uh oh!

cmonr commented May 31, 2018

Uh oh!

0xc0170 commented Jun 4, 2018

Uh oh!

0xc0170 commented Jun 5, 2018

Uh oh!

cmonr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jun 15, 2018

Uh oh!

cmonr commented Jun 19, 2018

Uh oh!

ithinuel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Jun 20, 2018

Uh oh!

0xc0170 commented Jun 27, 2018

Uh oh!

0xc0170 commented Jun 29, 2018

Uh oh!

0xc0170 commented Jun 29, 2018

Uh oh!

mbed-ci commented Jun 29, 2018

Triggering tests

Uh oh!

mbed-ci commented Jun 29, 2018

Uh oh!

mbed-ci commented Jun 29, 2018

Uh oh!

cmonr commented Jun 30, 2018

Uh oh!

0xc0170 commented Jul 2, 2018

Uh oh!

Uh oh!

0xc0170 Jun 14, 2018 •

edited

Loading

ithinuel left a comment •

edited

Loading

0xc0170 commented Jun 20, 2018 •

edited

Loading