Skip to content

resolve LWIP compiler warning #6780

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
Merged
merged 2 commits into from
May 3, 2018
Merged

resolve LWIP compiler warning #6780

merged 2 commits into from
May 3, 2018

Conversation

bmcdonnell-ionx
Copy link
Contributor

Description

Cast a variable to resolve a compiler warning.

Compile [ 12.3%]: lpc17_emac.c
[Warning] lpc17_emac.c@614,20: comparison between signed and unsigned integer expressions [-Wsign-compare]

There is probably a more straight-forward / maintainable way to do it, so feel free to improve on this PR.

Pull request type

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

0xc0170
0xc0170 previously approved these changes May 2, 2018
View reviewed changes
@0xc0170 0xc0170 requested a review from kjbracey May 2, 2018 11:52
kjbracey
kjbracey reviewed May 2, 2018
View reviewed changes
features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_NXP/TARGET_LPCTarget/lpc17_emac.c Outdated
for (idx = 0; idx < dn; idx++) {
/* It is safe to cast dn to uint32 here since it is either derived from the
* uint16 returned by pbuf_clen() above, or it is set to 1. */
for (idx = 0; idx < (u32_t) dn; idx++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can avoid a cast, would be sweet. In this case the problem is that this unsigned idx is intended for use elsewhere as the unsigned descriptor index, but this loop is actually comparing with the descriptor count.

No reason to use the same variable. Just have a local s_32_t count as the loop variable here.

@bmcdonnell-ionx bmcdonnell-ionx force-pushed the fix-lwip-compiler-warning branch from c696a14 to ba29bd9 Compare May 2, 2018 13:34
@bmcdonnell-ionx
Copy link
Contributor Author

@kjbracey-arm

No reason to use the same variable. Just have a local s_32_t count as the loop variable here.

Done.

kjbracey
kjbracey reviewed May 2, 2018
View reviewed changes
features/FEATURE_LWIP/lwip-interface/lwip-eth/arch/TARGET_NXP/TARGET_LPCTarget/lpc17_emac.c Outdated
@@ -611,7 +611,7 @@ static err_t lpc_low_level_output(struct netif *netif, struct pbuf *p)
/* Wait until enough descriptors are available for the transfer. */
/* THIS WILL BLOCK UNTIL THERE ARE ENOUGH DESCRIPTORS AVAILABLE */
#if NO_SYS == 0
for (idx = 0; idx < dn; idx++) {
for (count = 0; count < dn; count++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just declare it in here, please.

for (s_32_t count = 0; count < dn; count++)

Minimise variable scopes where possible to make clear they don't have any wider significance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is my preferred style, but I was following the style presented here. All local variables are declared at the top of the function.

That said, let me know if you still want me to make that change.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do like style consistency, but I like reduced-scope variables more. And as it's narrowly scoped, it has very little impact on global style...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@0xc0170
Copy link
Contributor

0xc0170 commented May 3, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented May 3, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented May 3, 2018

Exporter Build : SUCCESS

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

@mbed-ci
Copy link

mbed-ci commented May 3, 2018

Test : SUCCESS

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

@0xc0170
Copy link
Contributor

0xc0170 commented May 3, 2018

/morph mbed2-build

@bmcdonnell-ionx bmcdonnell-ionx changed the title cast to resolve compiler warning resolve compiler warning - lpc17_emac.c May 3, 2018
@bmcdonnell-ionx bmcdonnell-ionx changed the title resolve compiler warning - lpc17_emac.c resolve LWIP compiler warning May 3, 2018
@0xc0170 0xc0170 merged commit 2729c7a into ARMmbed:master May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjbracey kjbracey kjbracey left review comments

@0xc0170 0xc0170 0xc0170 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@bmcdonnell-ionx @0xc0170 @mbed-ci @kjbracey