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 : drivers/hal/platform folders update #7008

Merged
merged 4 commits into from Jul 2, 2018

Conversation

Projects
None yet
5 participants
@0xc0170
Member

0xc0170 commented May 24, 2018

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 0xc0170 requested review from ARMmbed/mbed-os-maintainers May 24, 2018

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

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

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 25, 2018

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

@cmonr

This comment has been minimized.

Contributor

cmonr commented May 31, 2018

Gonna need another rebase.

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

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_drivers branch from f96480a to abb16e4 Jun 4, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 4, 2018

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

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 5, 2018

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.

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

This comment has been minimized.

@cmonr

cmonr Jun 13, 2018

Contributor

Do case statements need braces?

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

This comment has been minimized.

@0xc0170

0xc0170 Jun 14, 2018

Member

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).

delete _chains[i];
}

This comment has been minimized.

@cmonr

cmonr Jun 13, 2018

Contributor

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

This comment has been minimized.

@0xc0170

0xc0170 Jun 14, 2018

Member

Part of NULL check, line 79

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

This comment has been minimized.

@cmonr

cmonr Jun 13, 2018

Contributor

This brace set feels like it should have an intentation.

This comment has been minimized.

@0xc0170

0xc0170 Jun 14, 2018

Member

function body, no indentation needed

}
}
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 comment has been minimized.

@cmonr

cmonr Jun 13, 2018

Contributor

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.

This comment has been minimized.

@0xc0170

0xc0170 Jun 14, 2018

Member

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 0xc0170 force-pushed the 0xc0170:fix_drivers branch from abb16e4 to c333eb5 Jun 15, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 15, 2018

Rebased to resolve conflicts .

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 19, 2018

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

@0xc0170 0xc0170 requested a review from ARMmbed/mbed-os-core Jun 19, 2018

@ithinuel

ithinuel suggested changes Jun 20, 2018 edited

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

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2018

@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 0xc0170 force-pushed the 0xc0170:fix_drivers branch from c333eb5 to 74af4e8 Jun 20, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 20, 2018

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

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_drivers branch from 74af4e8 to 3694237 Jun 27, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 27, 2018

Rebased again ! up to date

@0xc0170 0xc0170 force-pushed the 0xc0170:fix_drivers branch from 3694237 to ffcb6ec Jun 29, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 29, 2018

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jun 29, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 29, 2018

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

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 30, 2018

@ithinuel Are you all good with the changes?

@cmonr

cmonr approved these changes Jun 30, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 2, 2018

@cmonr I spoke with @ithinuel , all good here

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

14 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, 790 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10240 cycles (+1302 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment