Skip to content

Suppress shift warning for IAR compiler as well #7264

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

Merged
merged 1 commit into from
Jun 20, 2018

Conversation

deepikabhavnani
Copy link

Description

Compiler gives invalid warning for shift operation, more details at https://github.com/ARMmbed/mbed-os/blob/master/drivers/MbedCRC.h#L23

Suppressing the warning for IAR compiler as well.

Pull request type

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

Issue: #7251

Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

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

LGTM.

@@ -32,6 +32,8 @@ but we check for ( width < 8) before performing shift, so it should not be an is
#elif defined ( __GNUC__ )
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wshift-count-negative"
#elif defined (__ICCARM__)
#pragma diag_suppress=Pe062 // Shift count is negative
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 for the comment.

@cmonr
Copy link
Contributor

cmonr commented Jun 20, 2018

/morph build

@cmonr
Copy link
Contributor

cmonr commented Jun 20, 2018

@ARMmbed/mbed-os-maintainers So far, this is the second PR withing the last couple of days that I've seen resort to disabling warnings instead of updating code. It's fine, but it feels a bit risky imo.

Do we want to start doing something different in regards to these types of PRs, or just continue as is?

@mbed-ci
Copy link

mbed-ci commented Jun 20, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci
Copy link

mbed-ci commented Jun 20, 2018

@kjbracey
Copy link
Contributor

You've got to look at it case-by-case.

This one is absolutely a useful warning, so you definitely wouldn't want to disable it globally, but the particular code technique used here - achieving effective conditional compilation via if - triggers it. There's already a kill of the same warning present for another toolchain there, and the code is elegant and clear, and I wouldn't want to change it just to satisfy a machine that generates a spurious warning. (The observed negative shift can never actually be executed, so the total code is legal, but the warning is generated in the expression parser before it figures out the flowgraph).

My general feeling is that if the code gets less elegant or you start being coerced into poorer coding style because of a purely-advisory warning, the warning should go. Whether that's killed locally or globally depends on general utility of the warning versus how common false positives are.

Compilers can generate so many possible warnings, but their utility can range from "C required diagnostic, cos this isn't valid C" (so why not an error?) to "I've detected a real logic anomaly" (thanks!) to "this C code wouldn't be valid C++" (so?).

@mbed-ci
Copy link

mbed-ci commented Jun 20, 2018

@cmonr cmonr merged commit 585558c into ARMmbed:master Jun 20, 2018
@deepikabhavnani deepikabhavnani deleted the crc_warn branch June 20, 2018 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants